New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notification Module #270

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
2 participants
@ad-os
Contributor

ad-os commented Jan 3, 2017

This module will allow developers to integrate notifications with their functionality. Here are some wikis on which you can read more about this module.

  1. Notification Module Design
  2. Notification API Developer User Guide
  3. Configure Notifications-End User guide
@@ -90,6 +90,10 @@ dependencies {
[group: 'org.apache.httpcomponents', name: 'httpclient', version: '4.3.5'],
// Once we've switched to Java 8 this dep can be removed.
//[group: 'com.google.code.findbugs', name: 'jsr305', version: '3.0.0']
[group: 'org.springframework', name:'spring-jms'],
[group: 'org.apache.activemq', name: 'activemq-broker'],
[group: 'com.mockrunner', name: 'mockrunner-jms', version: '1.0.6'],

This comment has been minimized.

@nazeer1100126

nazeer1100126 Jan 13, 2017

Contributor

Are these dependencies should be part of testCompile?

@nazeer1100126

nazeer1100126 Jan 13, 2017

Contributor

Are these dependencies should be part of testCompile?

@@ -91,6 +91,11 @@ dependencies {
[group: 'org.apache.httpcomponents', name: 'httpclient', version: '4.3.5'],
// Once we've switched to Java 8 this dep can be removed.
//[group: 'com.google.code.findbugs', name: 'jsr305', version: '3.0.0']
[group: 'org.springframework', name:'spring-jms'],
[group: 'org.apache.activemq', name: 'activemq-broker'],
[group: 'com.mockrunner', name: 'mockrunner-jms', version: '1.0.6'],

This comment has been minimized.

@nazeer1100126

nazeer1100126 Jan 13, 2017

Contributor

Are these dependencies should be part of testCompile?

@nazeer1100126

nazeer1100126 Jan 13, 2017

Contributor

Are these dependencies should be part of testCompile?

@Test
@SuppressWarnings("unchecked")
public void testNotificationRetrieval() {
HashMap<String, Object> response = (HashMap<String, Object>) NotificationHelper.getNotifications(this.requestSpec,

This comment has been minimized.

@nazeer1100126

nazeer1100126 Jan 13, 2017

Contributor

Where is the actual test cases for notifications? You are just verifying the response?
Or is there any limitation here?

@nazeer1100126

nazeer1100126 Jan 13, 2017

Contributor

Where is the actual test cases for notifications? You are just verifying the response?
Or is there any limitation here?

This comment has been minimized.

@ad-os

ad-os Jan 13, 2017

Contributor

I have included the test for sending and receiving of notifications in test/java/org/apache/fineract/notification

@ad-os

ad-os Jan 13, 2017

Contributor

I have included the test for sending and receiving of notifications in test/java/org/apache/fineract/notification

this.tenantIdentifier = tenantIdentifier;
}
public static class NotificationBuilder {

This comment has been minimized.

@nazeer1100126

nazeer1100126 Jan 13, 2017

Contributor

You need move this NotificationBuilder to separate java file

@nazeer1100126

nazeer1100126 Jan 13, 2017

Contributor

You need move this NotificationBuilder to separate java file

This comment has been minimized.

@ad-os

ad-os Jan 13, 2017

Contributor

The use of this static class is only in NotificationData class. I have used this static inner class to implement a builder pattern. Is it advisable to move to another file ?

@ad-os

ad-os Jan 13, 2017

Contributor

The use of this static class is only in NotificationData class. I have used this static inner class to implement a builder pattern. Is it advisable to move to another file ?

This comment has been minimized.

@nazeer1100126

nazeer1100126 Jan 23, 2017

Contributor

Is it good to have two public class definitions in same java file?

@nazeer1100126

nazeer1100126 Jan 23, 2017

Contributor

Is it good to have two public class definitions in same java file?

'}';
}
public static class NotificationGeneratorBuilder {

This comment has been minimized.

@nazeer1100126

nazeer1100126 Jan 13, 2017

Contributor

NotificationGeneratorBuilder should be separate java file as NotificationGenerator should follow SRP? NotificationGenerator name will confuse the users as it is not a generator but it is Entity.

@nazeer1100126

nazeer1100126 Jan 13, 2017

Contributor

NotificationGeneratorBuilder should be separate java file as NotificationGenerator should follow SRP? NotificationGenerator name will confuse the users as it is not a generator but it is Entity.

This comment has been minimized.

@ad-os

ad-os Jan 13, 2017

Contributor

The use of this static class is only in NotificationGenerator class. I have used this static inner class to implement a builder pattern. Is it advisable to move to another file?

@ad-os

ad-os Jan 13, 2017

Contributor

The use of this static class is only in NotificationGenerator class. I have used this static inner class to implement a builder pattern. Is it advisable to move to another file?

private boolean isRead;
@Column(name = "created_at")
private String createdAt;

This comment has been minimized.

@nazeer1100126

nazeer1100126 Jan 13, 2017

Contributor

createdAt should be a date object?

@nazeer1100126

nazeer1100126 Jan 13, 2017

Contributor

createdAt should be a date object?

This comment has been minimized.

@ad-os

ad-os Jan 13, 2017

Contributor

I have stored a formatted string of date object in the database i.e why I used String here.

@ad-os

ad-os Jan 13, 2017

Contributor

I have stored a formatted string of date object in the database i.e why I used String here.

isRead = read;
}
public static class NotificationMapperBuilder {

This comment has been minimized.

@nazeer1100126

nazeer1100126 Jan 13, 2017

Contributor

NotificationMapperBuilder should be a separate java file to follow SRP?

@nazeer1100126

nazeer1100126 Jan 13, 2017

Contributor

NotificationMapperBuilder should be a separate java file to follow SRP?

This comment has been minimized.

@ad-os

ad-os Jan 13, 2017

Contributor

The use of this static class is only in NotificationMapper class. I have used this static inner class to implement a builder pattern. Is it advisable to move to another file?

@ad-os

ad-os Jan 13, 2017

Contributor

The use of this static class is only in NotificationMapper class. I have used this static inner class to implement a builder pattern. Is it advisable to move to another file?

private String action;
@Column(name = "actor")
private String actor;

This comment has been minimized.

@nazeer1100126

nazeer1100126 Jan 13, 2017

Contributor

Actor should be an id as two persons can have same name?

@nazeer1100126

nazeer1100126 Jan 13, 2017

Contributor

Actor should be an id as two persons can have same name?

This comment has been minimized.

@ad-os

ad-os Jan 14, 2017

Contributor

Refactored actor to an id :)

@ad-os

ad-os Jan 14, 2017

Contributor

Refactored actor to an id :)

if (notificationData.getUserId() == null) {
userIds = notificationData.getUserIds();
} else {
userIds = new ArrayList<>();

This comment has been minimized.

@nazeer1100126

nazeer1100126 Jan 13, 2017

Contributor

Is there any chance that userId is null but userIds is not null in notificationData?

@nazeer1100126

nazeer1100126 Jan 13, 2017

Contributor

Is there any chance that userId is null but userIds is not null in notificationData?

This comment has been minimized.

@ad-os

ad-os Jan 14, 2017

Contributor

I have used two different variables, one for when the notification is sent to only one user and another variable when notification is sent to more than one users. I will refactor it to use only one variable :)

@ad-os

ad-os Jan 14, 2017

Contributor

I have used two different variables, one for when the notification is sent to only one user and another variable when notification is sent to more than one users. I will refactor it to use only one variable :)

@ad-os ad-os closed this Jan 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment