Polymorphic event notification model in Rails

Question

I have a polymorphic notification model. When user gets a notification he can get on the page of the notifiable to check out what happened exactly. For instance invited you to join product or commented on a post. On the top of this I also have a polymorphic comment model. So user can get a notification not only for commented on a post but also for commented on a product customeror commented on a product, which of course just makes the whole situation more complex.

So the flow is the following:

If the user clicks on the notification, the link redirects to the custom checking_decreasing action to check the notification and decrease the notification number if necessary, and then it redirects to the desired page with anchor.

As you see my notification_redirection_path action looks terrible and I'm not even sure if it should be in application controller. Any ideas how I could refactor this code?

notifications index page (there is also a json version for dropdown notifications)

<%= link_to checking_decreasing_user_notifications_path(
 current_user,
 notifiable_type: notification.notifiable_type, 
 notifiable_id: notification.notifiable_id, 
 notification_action: notification.action
 ) do %>

notifications_controller

def checking_decreasing
 current_user.decreasing_comment_notification_number(
 params[:notifiable_type], 
 params[:notifiable_id]
 )
 redirect_to notification_redirection_path(
 params[:notifiable_type], 
 params[:notifiable_id], 
 params[:notification_action]
 )
end

application controller

def notification_redirection_path(notifiable_type, notifiable_id, action)
 if action == "commented"
 if notifiable_type == "ProductCustomer"
 product_customer = ProductCustomer.find(notifiable_id)
 product_id = product_customer.product_id
 elsif notifiable_type == "ProductLead"
 product_lead = ProductLead.find(notifiable_id)
 product_id = product_lead.product_id
 end
 route = case notifiable_type
 when "Post"
 posts_path(anchor: "post_#{notifiable_id}")#{}"/posts#post_#{notifiable_id}"
 when "Product"
 product_path(notifiable_id, anchor: "comment-panel")#/products/#{notifiable_id}#comment-panel"
 when "ProductLead"
 product_product_lead_path(product_id, notifiable_id, anchor: "comment-panel")#{}"/products/#{product_id}/#{notifiable_type}/#{notifiable_id}#comment-panel"
 when "ProductCustomer"
 product_product_customer_path(product_id, notifiable_id, anchor: "comment-panel") #/products/#{product_id}/#{notifiable_type}/#{notifiable_id}#comment-panel"
 end
 elsif action == "invited"
 product_path(notifiable_id, anchor: "product-invitation-well")
 elsif action == "accepted"
 product_product_users_path(notifiable_id)
 end
end

Answer 1

Here's a really basic refactor of notification_redirection_path:

def notification_redirection_path(notifiable_type:, notifiable_id:, notification_action:, **)
 case notification_action
 when "commented"
 case notifiable_type
 when "Post"
 posts_path(anchor: "post_#{notifiable_id}")
 when "Product"
 product_path(notifiable_id, anchor: "comment-panel")
 when "ProductLead"
 product_lead = ProductLead.find(notifiable_id)
 product_product_lead_path(product_lead.product_id, notifiable_id, anchor: "comment-panel")
 when "ProductCustomer"
 product_customer = ProductCustomer.find(notifiable_id)
 product_product_customer_path(product_customer.product_id, notifiable_id, anchor: "comment-panel")
 end
 when "invited"
 product_path(notifiable_id, anchor: "product-invitation-well")
 when "accepted"
 product_product_users_path(notifiable_id)
 end
end

A few things to notice:

1. I changed the method arguments to keyword arguments. This way instead of doing this:

   redirect_to notification_redirection_path(
    params[:notifiable_type], 
    params[:notifiable_id], 
    params[:notification_action]
   )
   

   ...we can do this:

   redirect_to notification_redirection_path(params)
   

2. I turned the outer if expression into a case expression. I also changed the indentation so the whens line up with their case. Usually I prefer the other style (like you wrote it) but I find with nested cases this is a bit easier to read.

3. I removed your initial if notifier_type == ... expression and moved that logic into the when "ProductLead" and when "ProductCustomer" blocks below. No need to have those in two separate places.

This is still pretty messy, though. A quick win for readability, maintainability, and testability would be to split it into separate methods:

def notification_redirection_path(notifiable_type:, notifiable_id:, notification_action: nil, **)
 case notifiable_type
 when "commented"
 commented_notification_redirection_path(notifiable_type, notifiable_id)
 when "invited"
 product_path(notifiable_id, anchor: "product-invitation-well")
 when "accepted"
 product_product_users_path(notifiable_id)
 end
end
private
def commented_notification_redirection_path(type, id)
 case type
 when "Post"
 posts_path(anchor: "post_#{id}")
 when "Product"
 product_path(id, anchor: "comment-panel")
 when "ProductLead"
 product_lead = ProductLead.find(id)
 product_product_lead_path(product_lead.product_id, id, anchor: "comment-panel")
 when "ProductCustomer"
 product_customer = ProductCustomer.find(id)
 product_product_customer_path(product_customer.product_id, id, anchor: "comment-panel")
 end
end

You could employ polymorphic_path to take this to its extreme conclusion, which might look something like this:

ACTION_ANCHORS = {
 "commented" => "comment-panel",
 "invited" => "product-invitation-well"
}
def notification_redirection_path(notifiable_type:, notifiable_id:, notification_action:, **)
 if notification_action == "commented" && notifiable_type == "Product"
 return posts_path(anchor: "post_#{notifiable_id}")
 end
 notifiable = notifiable_type.constantize.find(notifiable_id)
 path =
 if notification_action == "commented" &&
 (ProductLead === notifiable || ProductCustomer === notifiable)
 [ notifiable.product, notifiable ]
 elsif notification_action == "accepted"
 [ notifiable, :product_user ]
 else
 [ notifiable ]
 end
 polymorphic_path(path,
 anchor: ACTION_ANCHORS[notification_action])
end

...but it's probably not worth it, since you lose a lot in readability and maintainability.