I'm looking to improve a piece of code I did. I have a notification system in place, it's working well, the problem is that's it's not DRY at all, and to maintain it's just a nightmare... I was looking for some help to get this done in a proper way and in the rails way.
Please don't be scared :)
Here is my code :
The controller :
class NotificationsController < ApplicationController
def mynotifications
@u_notifications_paginate = current_user.notifications.all.order('created_at DESC').paginate(:page => params[:page], :per_page => 10)
@u_notifications_paginate.update read: true
end
def destroy
if current_user
notif = current_user.notifications.find_by_notification_uid!(params[:id])
notif.destroy
redirect_to mynotifications_path(locale: I18n.locale)
else
redirect_to root_path(locale: I18n.locale)
end
end
end
My helper :
module NotificationsHelper
def user_notifications_number
user.notifications.where(read: false).count
end
def user_notifications_paginate
user.notifications.where(read: false).order('created_at DESC').last(3)
end
def user_notifications_exists?
user.notifications.where(read: false).exists?
end
def newmember_reason?
user.notifications.where("reason = ?", "New Member").exists?
end
def acceptgroup_reason?
user.notifications.where("reason = ?", "Welcome").exists?
end
def deniedgroup_reason?
user.notifications.where("reason = ?", "Denied").exists?
end
def new_ownership_change_reason?
user.notifications.where("reason = ?", "New Owner").exists?
end
def left_ownership_change_reason?
user.notifications.where("reason = ?", "Owner Left").exists?
end
def new_invitation_reason?
user.notifications.where("reason = ?", "Group Invitation").exists?
end
def invitation_accepted_reason?
user.notifications.where("reason = ?", "Accepted").exists?
end
def invitation_declined_reason?
user.notifications.where("reason = ?", "Declined").exists?
end
end
My view :
<% @u_notifications_paginate.each do |notif| %>
<% if deniedgroup_reason? || invitation_declined_reason? %>
<div class="notif-negative">
<% elsif new_ownership_change_reason? || left_ownership_change_reason? %>
<div class="notif-info">
<% else %>
<div class="notif-positive">
<% end %>
<h6 class="d-inline"><strong><%= notif.reason %></strong></h6>
<div class="float-right d-inline"><%= notif.created_at.strftime("%A the %d/%m/%Y at %H:%M%p") %></div></br></br>
<% if newmember_reason? %>
<div class="d-inline"><strong><%= notif.notified_by.name %> <%= notif.notified_by.firstname %></strong> want's to join the Group : <strong>'<%= notif.group.name %>'</strong>.</div>
<%= link_to deny_member_path(guid: notif.notified_by, auth_token: notif.group.auth_token, locale: I18n.locale), class: 'btn btn-danger float-right' do %>
<i class="fa fa-times"></i>
<% end %>
<%= link_to accept_member_path(guid: notif.notified_by, auth_token: notif.group.auth_token, locale: I18n.locale), class: 'btn btn-success float-right' do %>
<i class="fa fa-check"></i>
<% end %>
<% elsif acceptgroup_reason? %>
<div class="d-inline">You have been accepted by <strong><%= notif.group.owner.firstname %></strong> in the Group : <strong>'<%= notif.group.name %>'</strong>.</div>
<%= link_to notification_path(id: notif.notification_uid, locale: I18n.locale), method: :delete, class: "btn btn-success float-right" do %>
<i class="fa fa-check"></i>
<% end %>
<% elsif deniedgroup_reason? %>
<div class="d-inline">You have been refused to enter the <strong>Group</strong>. 'Token' (Share Key) : <%= notif.group.auth_token %>.</div>
<%= link_to notification_path(id: notif.notification_uid, locale: I18n.locale), method: :delete, class: "btn btn-danger float-right" do %>
<i class="fa fa-times"></i>
<% end %>
<% elsif new_ownership_change_reason? %>
<div class="d-inline"><strong><%= notif.notified_by.firstname %></strong> gave you the Ownership of the Group : <strong>'<%= notif.group.name %>'</strong>.</div>
<%= link_to notification_path(id: notif.notification_uid, locale: I18n.locale), method: :delete, class: "btn btn-info float-right" do %>
<i class="fa fa-check"></i>
<% end %>
<% elsif left_ownership_change_reason? %>
<div class="d-inline"><strong><%= notif.notified_by.firstname %></strong> left the Group : <strong>'<%= notif.group.name %>'</strong> you are the new Owner.</div>
<%= link_to notification_path(id: notif.notification_uid, locale: I18n.locale), method: :delete, class: "btn btn-info float-right" do %>
<i class="fa fa-check"></i>
<% end %>
<% elsif new_invitation_reason? %>
<div class="d-inline"><strong><%= notif.notified_by.firstname %></strong> invites you in the Group : <strong>'<%= notif.group.name %>'</strong>.</div>
<%= link_to deny_invitation_path(guid: notif.user, auth_token: notif.group.auth_token, locale: I18n.locale), class: 'btn btn-danger float-right' do %>
<i class="fa fa-times"></i>
<% end %>
<%= link_to accept_invitation_path(guid: notif.user, auth_token: notif.group.auth_token, locale: I18n.locale), class: 'btn btn-success float-right' do %>
<i class="fa fa-check"></i>
<% end %>
<% elsif invitation_accepted_reason? %>
<div class="d-inline"><strong><%= notif.notified_by.firstname %></strong> accepted the Invitation in the Group : <strong>'<%= notif.group.name %>'</strong>.</div>
<%= link_to notification_path(id: notif.notification_uid, locale: I18n.locale), method: :delete, class: "btn btn-success float-right" do %>
<i class="fa fa-check"></i>
<% end %>
<% elsif invitation_declined_reason? %>
<div class="d-inline"><strong><%= notif.notified_by.firstname %></strong> refused the Invitation in the Group : <strong>'<%= notif.group.name %>'</strong>.</div>
<%= link_to notification_path(id: notif.notification_uid, locale: I18n.locale), method: :delete, class: "btn btn-danger float-right" do %>
<i class="fa fa-times"></i>
<% end %>
<% end %>
</div></br>
<% end %>
My navbar :
<div class="dropdown-menu" aria-labelledby="userNotification">
<% user_notifications_paginate.each do |notif| %>
<div class="dropdown-item card-pad">
<h6 class="d-inline"><strong><%= notif.reason %></strong></h6>
<div class="float-right d-inline"><%= notif.created_at.strftime("%d/%m/%Y") %></div>
<% if newmember_reason? %>
<div><strong><%= notif.notified_by.firstname %></strong> want's to join the Group : <strong>'<%= notif.group.name %>'</strong>.</div>
<sub><%= link_to(t(".notif_readmore"), mynotifications_path(locale: I18n.locale)) %></sub>
<% elsif acceptgroup_reason? %>
<div>You have been accepted by <strong><%= notif.group.owner.firstname %></strong> in the Group : <strong>'<%= notif.group.name %>'</strong>.</div>
<sub><%= link_to(t(".notif_readmore"), mynotifications_path(locale: I18n.locale)) %></sub>
<% elsif deniedgroup_reason? %>
<div>You have been refused to enter the <strong>Group</strong>.</div>
<sub><%= link_to(t(".notif_readmore"), mynotifications_path(locale: I18n.locale)) %></sub>
<% elsif new_ownership_change_reason? %>
<div>You are the new Owner of the Group : <strong>'<%= notif.group.name %>'</strong>.</div>
<sub><%= link_to(t(".notif_readmore"), mynotifications_path(locale: I18n.locale)) %></sub>
<% elsif left_ownership_change_reason? %>
<div><strong><%= notif.notified_by.firstname %></strong> left the Group : <strong>'<%= notif.group.name %>'</strong> you are the new Owner.</div>
<sub><%= link_to(t(".notif_readmore"), mynotifications_path(locale: I18n.locale)) %></sub>
<% elsif new_invitation_reason? %>
<div><strong><%= notif.notified_by.firstname %></strong> invites you in the Group : <strong>'<%= notif.group.name %>'</strong>.</div>
<sub><%= link_to(t(".notif_readmore"), mynotifications_path(locale: I18n.locale)) %></sub>
<% elsif invitation_accepted_reason? %>
<div><strong><%= notif.notified_by.firstname %></strong> accepted the Invitation in the Group : <strong>'<%= notif.group.name %>'</strong>.</div>
<sub><%= link_to(t(".notif_readmore"), mynotifications_path(locale: I18n.locale)) %></sub>
<% elsif invitation_declined_reason? %>
<div><strong><%= notif.notified_by.firstname %></strong> refused the Invitation in the Group : <strong>'<%= notif.group.name %>'</strong>.</div>
<sub><%= link_to(t(".notif_readmore"), mynotifications_path(locale: I18n.locale)) %></sub>
<% end %>
<hr class="col-5">
</div>
<% end %>
<% if user_notifications_exists? %>
<div class="dropdown-divider"></div>
<%= link_to(t(".notif_showall"), mynotifications_path(locale: I18n.locale), class: 'dropdown-item center') %>
<% else %>
<div class="center"><%= t(".notif_no_notif") %></div>
<% end %>
</div>
I'm so ashame to show that really...
Anyway what I have here, is that basically if a user has a notification, a popup is showed in the navbar, you can then click on the notification and read it in the view. As soon as the user comes in the view the notification is read (toggle boolean).
To create a notification I use this :
Notification.create!(read: false, user_id: user.id, notified_by_id: group.owner.id, group_id: group.id, reason: "Welcome")
This piece of code for example is in my controller that handle the membership, this one, more specifically, is to add a member, and this notification is created to notify the user that he has been accepted in the group.
I think you can see my problem and it is quite obvious. I'm playing with the condition in my view to call the good "reason" and so, display the right text according to that. I'm repeating my code several times, it is really horrible, I know. My question is how could I improve that. I'm not looking for a straight up answer or code, I'm just wondering how I could do that better and if you guys have some tricks to make this able to maintain.
Thanks a lot.
Here's a simple yet effective idea: don't hardcode the link between notification and its html representation. Instead, let notifications themselves define how they should render. For example, let each notification have a field partial_name.
<Notification id: 1, partial_name: 'newmember', ...>
<Notification id: 2, partial_name: 'accept_group', ...>
then you extract the corresponding partials
<div><strong><%= notif.notified_by.firstname %></strong> want's to join the Group : <strong>'<%= notif.group.name %>'</strong>.</div>
<sub><%= link_to(t(".notif_readmore"), mynotifications_path(locale: I18n.locale)) %></sub>
then your loop becomes REALLY simple:
<% user_notifications_paginate.each do |notif| %>
<%= render partial: "notifications/types/#{notif.partial_name}", locals: { notif: notif } %>
<% end %>
I might have missed a few tiny details, but this is the general idea.