# Notify owner of post and other commenters

I have a rails4 app. When a user comments on a post it should send a notification to all the guys who commented on the post and to the post creator. I have a working method in the controller, but it's kinda ugly and I'd like to refactor it.

Could somebody tell me what the rails convention for refactoring this?

Comment belongs_to :post, post has_many :comments

create#comment action

if @post_comment.save
((@post.users + [@post.user]).uniq - [current_user]).each do |post_commenter|
Notification.create(recipient_id: post_commenter.id, sender_id: current_user.id, notifiable: @post_comment.post, action: "commented")
end
....
end


First of all I would add a method to Post:

# app/models/post.rb
recipients = [user]               # the author of the original post
recipients << users               # people that wrote a comment
recipients.delete(comment.user)   # writer of the new comment

recipients.uniq.each do |recipient|
recipient_id: recipient.id,
sender_id: comment.user.id,
notifiable: self,
action: 'commented'
)
end
end


And just call that method in your controller:

if @post_comment.save
#...
end

• spickermann, you mistyped it. you use comment.user instead of current_user. Could you correct it for other who may check this solution? I have 2 questions as well. 1. Is recipients.delete(current_user) better than recipients - [current_user] or it's just a matter of taste? 2. How do you decide on which instance you call the method from post, post_comment and current_user? Mar 22 '16 at 13:33
• I decided to add the method to Post and call it on @post, because it felt to my that belongs into post, because a post is the instant that should (or could) know about all users involved into this conversation. There is no need for a comment or a current_user to know about other commenters. I think you should not pass current_user to that method. A comment, should know who the author was and I guess comment.user equals current_user, doesn't it? I prefer delete over - because it reads better. Mar 22 '16 at 14:02
• So you think I always should use in these kind of cases object.user over current_user? I usually use current_user because it's more readable, but if you know any reason not to do so, then I will change it. Mar 22 '16 at 18:06
• current_user only exists in the context of a controller. Whereas object.user exists in the model. If we want to move this functionality from the controller into the model (following Skinny Controller, Fat Model pattern) than you have to use object.user and cannot use current_user. Mar 22 '16 at 19:30
• might also consider adding after_create :notify_others_about_new_comment and omitting that method call from the controller Mar 24 '16 at 20:18

As you tagged your question with "performance" and the notification creation process needs not to be very real-time, I think it's better to move it to the background. For example, you can create one or more processes which focus on creating notifications when they received messages. Rails application send messages to those processes through a message broker like RabbitMQ.

• Aetherus, at the moment I'm doing it real-time with pusher. Maybe it's not the best idea, but I wan't users to get the notification right away without page load. Mar 22 '16 at 18:10
• This feels very slim for an answer, can you expand on how that's an improvement over the existing solution? Mar 22 '16 at 22:09

I think there are two possible solutions: Using a service object, or adding this to your Post class and utilizing Active Record callbacks.

## Using a service object

As you'll see in a moment, there isn't much in the service object except a way to send these notifications. While there isn't much code, what you'll gain is decoupling code that interacts solely with your own system (your models and database) from code that interacts with outside resources (notifications).

First, to centralize the logic of finding the post users, without the post author, add a method to the Post class:

class Post < ActiveRecord::Base
def other_users
users - user
end
end


Then the service object:

class PostService
def add_comment(post, author, comment_text)
new_comment = post.comments.create user: author, text: comment_text
post.other_users.each { |user| CommentNotification.create user, new_comment }
new_comment
end
end


class CommentsController < ApplicationController
def create
post = Post.find params[:comment][:post_id]
author = User.find params[:comment][:user_id]
@comment = post_service.add_comment author, params[:comment][:text]

if post.save
redirect_to post_url(post)
else
render action: 'create'
end
end

private

def post_service
@post_service ||= PostService.new
end
end


## Using ActiveRecord callbacks

Honestly my initial reaction was to use an after_save callback, but I also read spickermann's comment about shying away from ActiveRecord callbacks. While I've had problems with callbacks before, if you construct your code correctly, the callback shouldn't be a problem. I also don't think the controller should know much of anything about posts, comments or notifications. The controller should handle the basics of the HTTP request, and little more.

Adding an add_comment method to the Post class would be the appropriate place to enforce this logic:

class Post < ActiveRecord::Base

def add_comment(author, text, notify_users = true)
new_comment = Comment.new(post: self, user: author, text: text)

other_users.each do |user|
end if notify_users

return new_comment
end

private

end

def other_users
users - user
end

end

end
end


The notify_users argument, true by default, allows you to disable notifications which solves Spickermann's major complaint about spamming notifications if you need to do a data import.

Aside: I took a few liberties with your other classes, so they might not exactly match your object model, but you should get the main idea.

• The users you want to notify are the users for the post, excluding the creator of the post. I think that is best encapsulated in its own method: other_users

• Create a class called CommentNotification which inherits from Notification, and place all logic for constructing that notification in there making this portable.

class CommentNotification < Notification

def self.create(user, comment)
base.create(
recipient_id: user.id,
sender_id: comment.post_user_id,
notifiable: comment.post,
end
end

• The controller becomes extremely slim:

class PostCommentsController < ApplicationController
def create
post = Post.find params[:comment][:post_id]
author = User.find params[:comment][:user_id]
@comment = post.add_comment author, params[:comment][:text]

if post.save
redirect_to post_url(post)
else
render action: 'create'
end
end
end

• Greg, you mentioned service objects, but as far as I see this is the callback solution. Could you show it with service object? I would avoid using callbacks. May 25 '16 at 20:47
• @SzilardMagyar: I added a service object example to my answer as well. May 26 '16 at 13:35