Merge pull request #815 from consul/notifications-tweaks

Notifications plus

app/controllers/comments_controller.rb

@@ -65,11 +65,11 @@ class CommentsController < ApplicationController
 
     def add_notification(comment)
       if comment.reply?
-        author = comment.parent.author
+        notifiable = comment.parent
       else
-        author = comment.commentable.author
+        notifiable = comment.commentable
       end
-      author.notifications.create!(notifiable: comment) unless comment.made_by? author
+      Notification.add(notifiable.author_id, notifiable) unless comment.author_id == notifiable.author_id
    end
 
 end

app/controllers/notifications_controller.rb

@@ -9,7 +9,7 @@ class NotificationsController < ApplicationController
 
   def show
     @notification = current_user.notifications.find(params[:id])
-    redirect_to url_for(@notification.notifiable.commentable)
+    redirect_to url_for(@notification.notifiable)
   end
 
   def mark_all_as_read

app/helpers/notifications_helper.rb

@@ -1,6 +1,6 @@
 module NotificationsHelper
 
   def notification_action(notification)
-    notification.notifiable.reply? ? "replied_to_your_comment" : "commented_on_your_debate"
+    notification.notifiable_type == "Comment" ? "replies_to" : "comments_on"
   end
 end

app/models/comment.rb

@@ -90,10 +90,6 @@ class Comment < ActiveRecord::Base
     !root?
   end
 
-  def made_by?(user)
-    self.user == user
-  end
-
   def call_after_commented
     self.commentable.try(:after_commented)
   end

app/models/notification.rb

@@ -1,14 +1,10 @@
 class Notification < ActiveRecord::Base
-  belongs_to :user
+  belongs_to :user, counter_cache: true
   belongs_to :notifiable, polymorphic: true
 
   scope :unread, -> { all }
   scope :recent, -> { order(id: :desc) }
-  scope :for_render, -> { includes(notifiable: [:user]) }
-
-  def username
-    notifiable.user.username
-  end
+  scope :for_render, -> { includes(:notifiable) }
 
   def timestamp
     notifiable.created_at
@@ -17,4 +13,12 @@ class Notification < ActiveRecord::Base
   def mark_as_read
     self.destroy
   end
+
+  def self.add(user_id, notifiable)
+    if notification = Notification.find_by(user_id: user_id, notifiable: notifiable)
+      Notification.increment_counter(:counter, notification.id)
+    else
+      Notification.create!(user_id: user_id, notifiable: notifiable)
+    end
+  end
 end

app/views/comments/_comment.html.erb

@@ -71,7 +71,7 @@
             <%= render 'comments/votes', comment: comment %>
           </span>
 
-          <div class="reply">
+          <div id="<%= dom_id(comment) %>_reply" class="reply">
             <%= t("comments.comment.responses", count: child_comments_of(comment).size) %>
 
             <% if user_signed_in? %>

app/views/devise/menu/_login_items.html.erb

@@ -2,9 +2,9 @@
   <% if user_signed_in? %>
     <li>
       <%= link_to notifications_path, class: "notifications" do %>
-        <% if current_user.notifications.count > 0 %>
+        <% if current_user.notifications_count > 0 %>
           <i class="icon-circle"></i>
-          <i class="icon-notification" title="<%= t('layouts.header.new_notifications', count: current_user.notifications.count).html_safe %>">
+          <i class="icon-notification" title="<%= t('layouts.header.new_notifications', count: current_user.notifications_count).html_safe %>">
           </i>
         <% else %>
           <i class="icon-no-notification" title="<%= t('layouts.header.no_notifications') %>"></i>

app/views/notifications/_notification.html.erb

@@ -1,9 +1,8 @@
 <li id="<%= dom_id(notification) %>" class="notification">
   <%= link_to notification do %>
     <p>
-      <strong><%= notification.username %></strong>
-      <em><%= t("notifications.index.#{notification_action(notification)}") %></em>
-      <strong><%= notification.notifiable.commentable.title %></strong>
+      <em><%= t("notifications.index.#{notification_action(notification)}", count: notification.counter) %></em>
+      <strong><%= notification.notifiable.is_a?(Comment) ? notification.notifiable.commentable.title : notification.notifiable.title %></strong>
     </p>
     <p class="time"><%= l notification.timestamp, format: :datetime %></p>
   <% end %>

config/i18n-tasks.yml

@@ -127,8 +127,8 @@ ignore_unused:
   - 'proposals.index.select_order'
   - 'proposals.index.orders.*'
   - 'proposals.index.search_form.*'
-  - 'notifications.index.commented_on_your_debate'
-  - 'notifications.index.replied_to_your_comment'
+  - 'notifications.index.comments_on*'
+  - 'notifications.index.replies_to*'
   - 'helpers.page_entries_info.*' # kaminari
   - 'views.pagination.*' # kaminari
 # - '{devise,kaminari,will_paginate}.*'

config/locales/en.yml

@@ -313,8 +313,12 @@ en:
     index:
       mark_all_as_read: "Mark all as read"
       empty_notifications: "You don't have new notifications."
-      commented_on_your_debate: "commented on your debate"
-      replied_to_your_comment: "replied to your comment on"
+      comments_on:
+        one: "Someone commented on"
+        other: "There are %{count} new comments on"
+      replies_to:
+        one: "Someone replied to your comment on"
+        other: "There are %{count} new replies to your comment on"
   simple_captcha:
     placeholder: "Enter the text from the image"
     label: "Enter the text from the image in the box below"

config/locales/es.yml

@@ -313,8 +313,12 @@ es:
     index:
       mark_all_as_read: "Marcar todas como leídas"
       empty_notifications: "No tienes notificaciones nuevas."
-      commented_on_your_debate: "ha comentado en tu debate"
-      replied_to_your_comment: "ha respondido a tu comentario en"
+      comments_on:
+        one: "Hay un nuevo comentario en"
+        other: "Hay %{count} comentarios nuevos en"
+      replies_to:
+        one: "Hay una respuesta nueva a tu comentario en"
+        other: "Hay %{count} nuevas respuestas a tu comentario en"
   simple_captcha:
     placeholder: "Introduce el texto de la imagen"
     label: "Introduce el texto de la imagen en la siguiente caja"

db/migrate/20160108114750_add_counter_to_notifications.rb

@@ -0,0 +1,5 @@
+class AddCounterToNotifications < ActiveRecord::Migration
+  def change
+    add_column :notifications, :counter, :integer, default: 1
+  end
+end

db/migrate/20160108133501_add_notifications_counter_cache_to_user.rb

@@ -0,0 +1,5 @@
+class AddNotificationsCounterCacheToUser < ActiveRecord::Migration
+  def change
+    add_column :users, :notifications_count, :integer, default: 0
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20160105170113) do
+ActiveRecord::Schema.define(version: 20160108133501) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -208,6 +208,7 @@ ActiveRecord::Schema.define(version: 20160105170113) do
     t.integer "user_id"
     t.integer "notifiable_id"
     t.string  "notifiable_type"
+    t.integer "counter",         default: 1
   end
 
   add_index "notifications", ["user_id"], name: "index_notifications_on_user_id", using: :btree
@@ -338,6 +339,7 @@ ActiveRecord::Schema.define(version: 20160105170113) do
     t.datetime "erased_at"
     t.boolean  "public_activity",                      default: true
     t.boolean  "newsletter",                           default: false
+    t.integer  "notifications_count",                  default: 0
   end
 
   add_index "users", ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true, using: :btree

spec/factories.rb

@@ -292,7 +292,7 @@ FactoryGirl.define do
 
   factory :notification do
     user
-    association :notifiable, factory: :comment
+    association :notifiable, factory: :proposal
   end
 
 end

spec/features/notifications_spec.rb

@@ -4,6 +4,7 @@ feature "Notifications" do
   let(:author) { create :user }
   let(:user) { create :user }
   let(:debate) { create :debate, author: author }
+  let(:proposal) { create :proposal, author: author }
 
   scenario "User commented on my debate", :js do
     login_as user
@@ -18,12 +19,44 @@ feature "Notifications" do
     logout
     login_as author
     visit root_path
-    expect(page).to have_xpath "//a[@class='with_notifications' and @href='#{notifications_path}' and text()='Notificaciones']"
 
-    click_link "Notificaciones"
+    find(".icon-notification").click
+
     expect(page).to have_css ".notification", count: 1
-    expect(page).to have_content user.username
-    expect(page).to have_content "commented on your debate"
+
+    expect(page).to have_content "Someone commented on"
+    expect(page).to have_xpath "//a[@href='#{notification_path(Notification.last)}']"
+  end
+
+  scenario "Multiple comments on my proposal", :js do
+    login_as user
+    visit proposal_path proposal
+
+    fill_in "comment-body-proposal_#{proposal.id}", with: "I agree"
+    click_button "Publish comment"
+    within "#comments" do
+      expect(page).to have_content "I agree"
+    end
+
+    logout
+    login_as create(:user)
+    visit proposal_path proposal
+
+    fill_in "comment-body-proposal_#{proposal.id}", with: "I disagree"
+    click_button "Publish comment"
+    within "#comments" do
+      expect(page).to have_content "I disagree"
+    end
+
+    logout
+    login_as author
+    visit root_path
+
+    find(".icon-notification").click
+
+    expect(page).to have_css ".notification", count: 1
+
+    expect(page).to have_content "There are 2 new comments on"
     expect(page).to have_xpath "//a[@href='#{notification_path(Notification.last)}']"
   end
 
@@ -45,12 +78,39 @@ feature "Notifications" do
     logout
     login_as author
     visit root_path
-    expect(page).to have_xpath "//a[@class='with_notifications' and @href='#{notifications_path}' and text()='Notificaciones']"
 
-    visit notifications_path
+    find(".icon-notification").click
+
     expect(page).to have_css ".notification", count: 1
-    expect(page).to have_content user.username
-    expect(page).to have_content "replied to your comment on"
+    expect(page).to have_content "Someone replied to your comment on"
+    expect(page).to have_xpath "//a[@href='#{notification_path(Notification.last)}']"
+  end
+
+  scenario "Multiple replies to my comment", :js do
+    comment = create :comment, commentable: debate, user: author
+    3.times do |n|
+      login_as create(:user)
+      visit debate_path debate
+
+      within("#comment_#{comment.id}_reply") { click_link "Reply" }
+      within "#js-comment-form-comment_#{comment.id}" do
+        fill_in "comment-body-comment_#{comment.id}", with: "Reply number #{n}"
+        click_button "Publish reply"
+      end
+
+      within "#comment_#{comment.id}" do
+        expect(page).to have_content "Reply number #{n}"
+      end
+      logout
+    end
+
+    login_as author
+    visit root_path
+
+    find(".icon-notification").click
+
+    expect(page).to have_css ".notification", count: 1
+    expect(page).to have_content "There are 3 new replies to your comment on"
     expect(page).to have_xpath "//a[@href='#{notification_path(Notification.last)}']"
   end
 
@@ -63,9 +123,8 @@ feature "Notifications" do
     within "#comments" do
       expect(page).to have_content "I commented on my own debate"
     end
-    expect(page).to have_xpath "//a[@class='without_notifications' and @href='#{notifications_path}' and text()='Notificaciones']"
 
-    click_link "Notificaciones"
+    find(".icon-no-notification").click
     expect(page).to have_css ".notification", count: 0
   end
 
@@ -83,7 +142,8 @@ feature "Notifications" do
     within "#comment_#{comment.id}" do
       expect(page).to have_content "I replied to my own comment"
     end
-    expect(page).to have_xpath "//a[@class='without_notifications' and @href='#{notifications_path}' and text()='Notificaciones']"
+
+    find(".icon-no-notification")
 
     visit notifications_path
     expect(page).to have_css ".notification", count: 0
@@ -126,7 +186,7 @@ feature "Notifications" do
     login_as user
     visit notifications_path
 
-    expect(page).to have_content "There are no new notifications"
+    expect(page).to have_content "You don't have new notifications"
   end
 
 end

spec/helpers/notifications_helper_spec.rb

@@ -5,38 +5,21 @@ describe NotificationsHelper do
   describe "#notification_action" do
     let(:debate) { create :debate }
     let(:debate_comment) { create :comment, commentable: debate }
-    let(:comment_reply)  { create :comment, commentable: debate, parent: debate_comment }
 
     context "when action was comment on a debate" do
       it "returns correct text when someone comments on your debate" do
-        notification = create :notification, notifiable: debate_comment
-        expect(notification_action(notification)).to eq "commented_on_your_debate"
+        notification = create :notification, notifiable: debate
+        expect(notification_action(notification)).to eq "comments_on"
       end
     end
 
     context "when action was comment on a debate" do
       it "returns correct text when someone replies to your comment" do
-        notification = create :notification, notifiable: comment_reply
-        expect(notification_action(notification)).to eq "replied_to_your_comment"
+        notification = create :notification, notifiable: debate_comment
+        expect(notification_action(notification)).to eq "replies_to"
       end
     end
   end
 
-  describe "#notifications_class_for" do
-    let(:user) { create :user }
-
-    context "when user doesn't have notifications" do
-      it "returns class 'without_notifications'" do
-        expect(notifications_class_for(user)).to eq "without_notifications"
-      end
-    end
-
-    context "when user has notifications" do
-      it "returns class 'with_notifications'" do
-        notification = create :notification, user: user
-        expect(notifications_class_for(user)).to eq "with_notifications"
-      end
-    end
-  end
 
 end

spec/models/comment_spec.rb

@@ -129,18 +129,4 @@ describe Comment do
     end
   end
 
-  describe "#made_by?" do
-    let(:author)  { create :user }
-    let(:comment) { create :comment, user: author }
-
-    it "returns true if comment was made by user" do
-      expect(comment.made_by?(author)).to be true
-    end
-
-    it "returns false if comment was not made by user" do
-      not_author = create :user
-      expect(comment.made_by?(not_author)).to be false
-    end
-  end
-
 end

spec/models/notification_spec.rb

@@ -23,7 +23,7 @@ describe Notification do
 
   describe "#for_render (scope)" do
     it "returns notifications including notifiable and user" do
-      expect(Notification).to receive(:includes).with(notifiable: [:user]).exactly(:once)
+      expect(Notification).to receive(:includes).with(:notifiable).exactly(:once)
       Notification.for_render
     end
   end
@@ -47,12 +47,4 @@ describe Notification do
     end
   end
 
-  describe "#username" do
-    it "returns the username of the activity's author" do
-      comment = create :comment
-      notification = create :notification, notifiable: comment
-      expect(notification.username).to eq comment.author.username
-    end
-  end
-
 end