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

Background Job support #29

Closed
sumitngupta opened this issue Jun 14, 2017 · 7 comments
Closed

Background Job support #29

sumitngupta opened this issue Jun 14, 2017 · 7 comments

Comments

@sumitngupta
Copy link

Hello!

First off, the gem looks amazing. We just ran through a test integration and we're ecstatic that it covered all our needs and more. We're going to be fully integrating it this week but one thing we noticed was that when a notification is triggered its done entirely inline. This would fall over performance wise for us since a single comment could cause hundreds of notification records inserts. Looking through the API it looks pretty straight forward to integrate some ActiveJob queueing to handle the notification creation for these three methods:

https://github.com/simukappu/activity_notification/blob/master/lib/activity_notification/models/concerns/notifiable.rb#L240
https://github.com/simukappu/activity_notification/blob/master/lib/activity_notification/models/concerns/notifiable.rb#L260
https://github.com/simukappu/activity_notification/blob/master/lib/activity_notification/models/concerns/notifiable.rb#L280

I was going to create an option to enable / disable whether it used ActiveJob's or inline (as it currently is implemented).

First question... Is this something thats already in place but I'm just blind and missed it?

Second question in case the answer to the first is that its not in fact already in the gem.. is this something you'd want / appreciate as a pull request (presuming test coverage is in place and all the other responsible open source requirements are handled) ? I ask only because it might influence how we patch in the background job support.

Again, fantastic gem and looking forward to exploring all its goodies!

Cheers,
Sumit

@simukappu
Copy link
Owner

simukappu commented Jun 15, 2017

Hi @sumitngupta,

As activity_notification gem, currently background job API with ActiveJob is only supported for sending notification email. It is provided as send_later option of Notification API (notify method).

On the other hand, we can use background job tools such as delayed_job with activity_notification.
We can call Notification API with delay method, like this,

notifiable.delay.notify :users

Actually we are using activity_notification with delayed_job like this.

Of course, I would appreciated it if you could send us pull request to enhance "Asynchronous Notification API" with ActiveJob for better activity_notification.

Regards,
Shota

@adambedford
Copy link

adambedford commented Nov 30, 2017

@sumitngupta Did you get anywhere with this. I'd love to see this in the library too since creating notifications inline is a big performance issue.

Obviously you can get around it by setting tracked: false and implementing your own callbacks but it'd be a nice feature for the library!

@nikhilgoyal22
Copy link

Is there any work going on n this?

@simukappu
Copy link
Owner

Not yet. But I would like to make notify method work as active_job's job. This will not be a so big enhance. I would appreciate it if you could contribute this feature. I will also try do this when I have time.
Thanks!

@sumitngupta
Copy link
Author

Apologies @adambedford for completely missing the question to me. We moved away from ActivityNotification in the end since our domain problem required something slightly different (stupid domain complexities). We ended up using the ".delay" method until we moved over to our own notifications system.

@simukappu
Copy link
Owner

Hi,
I have added native support for asynchronous notification API using ActiveJob to improve application performance.
Does anyone who can try this new function from development branch?

See the following REDME to try it.
Asynchronous notification API with ActiveJob
https://github.com/simukappu/activity_notification/blob/development/README.md#asynchronous-notification-api-with-activejob
Automatic tracked notifications
https://github.com/simukappu/activity_notification/blob/development/README.md#automatic-tracked-notifications

Thanks!

@simukappu
Copy link
Owner

Just released as new version 1.7.0.

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

No branches or pull requests

4 participants