Skip to content
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

Added notification module #178

Closed
wants to merge 1 commit into from
Closed

Conversation

adhyans
Copy link
Contributor

@adhyans adhyans commented Jul 19, 2016

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

@adhyans adhyans force-pushed the notification branch 2 times, most recently from 00f925a to 2f01e13 Compare July 22, 2016 07:12
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-2.10-bin.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-2.10-all.zip
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need this change?

@@ -75,6 +75,15 @@ public static SearchParameters forClients(final String sqlSearch, final Long off
orderBy, sortOrder, staffId, accountNo, loanId, savingsId, orphansOnly, isSelfUser);
}

public static SearchParameters forNotifications(final Integer offset, final Integer limit,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new method is not required. You can use SearchParameters.forPagination(...) method

@@ -167,6 +168,12 @@ protected void onSuccessfulAuthentication(HttpServletRequest request,
throws IOException {
super.onSuccessfulAuthentication(request, response, authResult);
AppUser user = (AppUser) authResult.getPrincipal();

if (notificationReadPlatformService.hasUnreadNotifications(user.getId())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is notification module supported only for basicauth profile? Don't we support for oauth profile?

import javax.ws.rs.core.UriInfo;

@Path("/notifications")
@Component
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scope should be singleton

@@ -39,5 +39,5 @@ INSERT INTO `stretchy_report_parameter` (`report_id`, `parameter_id`, `report_pa
INSERT INTO `stretchy_report_parameter` (`report_id`, `parameter_id`, `report_parameter_name`) VALUES
((select sr.id from stretchy_report sr where sr.report_name='GeneralLedgerReport'),
(select sp.id from stretchy_parameter sp where sp.parameter_name='OfficeIdSelectOne'),
'office');
'officeId');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not modify the existing migration files

@@ -45,13 +45,16 @@
import org.apache.fineract.infrastructure.core.serialization.ApiRequestJsonSerializationSettings;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Listen to the business event and publish the notification event

@@ -53,7 +53,9 @@
import org.apache.fineract.infrastructure.core.serialization.ToApiJsonSerializer;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Listen to the business event and publish the notification event

@@ -54,7 +54,9 @@
import org.apache.fineract.infrastructure.core.serialization.ToApiJsonSerializer;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Listen to the business event and publish the notification event

@@ -18,25 +18,6 @@
*/
package org.apache.fineract.portfolio.client.api;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Listen to the business event and publish the notification event

@@ -38,14 +38,18 @@
import org.apache.fineract.infrastructure.core.serialization.ApiRequestJsonSerializationSettings;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Listen to the business event and publish the notification event

@nazeer1100126
Copy link
Contributor

Can you please give more details why do we need to use spring-jms?

@adhyans
Copy link
Contributor Author

adhyans commented Nov 17, 2016

I have used jms to publish the generated notifications to a queue(implmented via ActiveMq) and then used the jmslistener to subscribe to that queue and listen for any new notification. More specifically, jms with ActiveMq has been used to make the generation and reception of messages to be asynchronous.
And Thanks for the review 😄 I will work on it.

@nazeer1100126
Copy link
Contributor

Have a look at how business events are implemented in Fineract. I believe the same way you can do it asynchronously by listening to required events. By this approach you no need to modify all API resources.
I hope I am clear enough to you :-)

@adhyans
Copy link
Contributor Author

adhyans commented Nov 17, 2016

I think I have followed the same design as the implementation of business events.

You no need to modify all API resources

I have used only the publish method call from the API resources like
this.publisher.publishEvent(Event Object) similar to what the business design uses i.e this.businessEventNotifierService.notifyBusinessEventToBeExecuted(some parameters)

Reasons why me and my mentor(Pranjal) decided to use an external queue :-

  1. if we don't use an external queue we can be at a disadvantage of loosing messages if the listener ceases to stop working.
  2. This design follows pub-sub pattern which is nowadays generally used to design notification module.

I think the only difference is that business event design uses a Map to store the list of published events and my implementation uses an external queue.
So, should I refactor like the one suggested by you or proceed with my implementation ?
Thanks.

@nazeer1100126
Copy link
Contributor

You need to listen to the business events and you can push notifications in your message queue. API resourced should not have this changes.

@adhyans
Copy link
Contributor Author

adhyans commented Nov 22, 2016

Okay, will do and get back to you.Thanks :)

@adhyans
Copy link
Contributor Author

adhyans commented Nov 29, 2016

Hi Nazeer,
What do you mean by

API resourced should not have this changes ?

What I have currently refactored is like initiating the business events in the respective API resources and then listening to those business events. Is it good enough ?

@nazeer1100126
Copy link
Contributor

Have a look at current business events implementation. They won't be triggered from api resources.

@adhyans adhyans closed this Jan 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants