From ab74bd58faefb6e473724f1f73b27e99783de46b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juanjo=20Baza=CC=81n?= Date: Fri, 8 Jan 2016 12:35:23 +0100 Subject: [PATCH 1/6] avoids queries when creating a notification --- app/controllers/comments_controller.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index c30ddbaf2..2ce536c5c 100644 --- a/app/controllers/comments_controller.rb +++ b/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.create!(user_id: notifiable.author_id, notifiable: comment) unless comment.author_id == notifiable.author_id end end From 790d8de5b6335f787ea9bcf533433431b61e7dbb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juanjo=20Baz=C3=A1n?= Date: Fri, 8 Jan 2016 12:35:56 +0100 Subject: [PATCH 2/6] removes unused method --- app/models/comment.rb | 4 ---- spec/models/comment_spec.rb | 14 -------------- 2 files changed, 18 deletions(-) diff --git a/app/models/comment.rb b/app/models/comment.rb index 44e16b729..3771af84a 100644 --- a/app/models/comment.rb +++ b/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 diff --git a/spec/models/comment_spec.rb b/spec/models/comment_spec.rb index 684de6438..73b70fb42 100644 --- a/spec/models/comment_spec.rb +++ b/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 From a976e16ef345acd744410fbeff9bc1b03f7c021b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juanjo=20Baz=C3=A1n?= Date: Fri, 8 Jan 2016 12:36:43 +0100 Subject: [PATCH 3/6] redirects notification show to notifiable --- app/controllers/notifications_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index 3820d7e36..a4ec31b50 100644 --- a/app/controllers/notifications_controller.rb +++ b/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 From 704a038795d2c9714ac0f6c6aaec308f24578555 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juanjo=20Baz=C3=A1n?= Date: Fri, 8 Jan 2016 14:32:16 +0100 Subject: [PATCH 4/6] groups notifications --- app/controllers/comments_controller.rb | 2 +- app/helpers/notifications_helper.rb | 2 +- app/models/notification.rb | 14 ++-- app/views/comments/_comment.html.erb | 2 +- .../notifications/_notification.html.erb | 5 +- config/locales/en.yml | 8 +- config/locales/es.yml | 8 +- ...0108114750_add_counter_to_notifications.rb | 5 ++ db/schema.rb | 3 +- spec/factories.rb | 2 +- spec/features/notifications_spec.rb | 84 ++++++++++++++++--- spec/helpers/notifications_helper_spec.rb | 25 +----- spec/models/notification_spec.rb | 10 +-- 13 files changed, 111 insertions(+), 59 deletions(-) create mode 100644 db/migrate/20160108114750_add_counter_to_notifications.rb diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index 2ce536c5c..b728ba729 100644 --- a/app/controllers/comments_controller.rb +++ b/app/controllers/comments_controller.rb @@ -69,7 +69,7 @@ class CommentsController < ApplicationController else notifiable = comment.commentable end - Notification.create!(user_id: notifiable.author_id, notifiable: comment) unless comment.author_id == notifiable.author_id + Notification.add(notifiable.author_id, notifiable) unless comment.author_id == notifiable.author_id end end diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb index 63618440b..281163380 100644 --- a/app/helpers/notifications_helper.rb +++ b/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 diff --git a/app/models/notification.rb b/app/models/notification.rb index e22e68ad2..7d3ebd146 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -4,11 +4,7 @@ class Notification < ActiveRecord::Base 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 \ No newline at end of file diff --git a/app/views/comments/_comment.html.erb b/app/views/comments/_comment.html.erb index 9178f0b9b..00d62ed5f 100644 --- a/app/views/comments/_comment.html.erb +++ b/app/views/comments/_comment.html.erb @@ -71,7 +71,7 @@ <%= render 'comments/votes', comment: comment %> -
+
<%= t("comments.comment.responses", count: child_comments_of(comment).size) %> <% if user_signed_in? %> diff --git a/app/views/notifications/_notification.html.erb b/app/views/notifications/_notification.html.erb index 7315f899d..28def42f7 100644 --- a/app/views/notifications/_notification.html.erb +++ b/app/views/notifications/_notification.html.erb @@ -1,9 +1,8 @@
  • <%= link_to notification do %>

    - <%= notification.username %> - <%= t("notifications.index.#{notification_action(notification)}") %> - <%= notification.notifiable.commentable.title %> + <%= t("notifications.index.#{notification_action(notification)}", count: notification.counter) %> + <%= notification.notifiable.is_a?(Comment) ? notification.notifiable.commentable.title : notification.notifiable.title %>

    <%= l notification.timestamp, format: :datetime %>

    <% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index e8d82bcf8..d04cba8fe 100755 --- a/config/locales/en.yml +++ b/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" diff --git a/config/locales/es.yml b/config/locales/es.yml index 6d6f9a72b..bef5af656 100644 --- a/config/locales/es.yml +++ b/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" diff --git a/db/migrate/20160108114750_add_counter_to_notifications.rb b/db/migrate/20160108114750_add_counter_to_notifications.rb new file mode 100644 index 000000000..7cf3c434c --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 35aa4b68c..1ba98a0a3 100644 --- a/db/schema.rb +++ b/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: 20160108114750) 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 diff --git a/spec/factories.rb b/spec/factories.rb index d48aba942..6e910a214 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -292,7 +292,7 @@ FactoryGirl.define do factory :notification do user - association :notifiable, factory: :comment + association :notifiable, factory: :proposal end end diff --git a/spec/features/notifications_spec.rb b/spec/features/notifications_spec.rb index 22e4b320d..1ffc6a854 100644 --- a/spec/features/notifications_spec.rb +++ b/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 diff --git a/spec/helpers/notifications_helper_spec.rb b/spec/helpers/notifications_helper_spec.rb index fa378b2ec..ccb3b3a9d 100644 --- a/spec/helpers/notifications_helper_spec.rb +++ b/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 diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index 0b8354e83..361211a51 100644 --- a/spec/models/notification_spec.rb +++ b/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 From 432e9e0d5bf0e64244c0d6d1c6145bc9bb98b84a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juanjo=20Baz=C3=A1n?= Date: Fri, 8 Jan 2016 14:39:34 +0100 Subject: [PATCH 5/6] adds counter cache for user's notifications --- app/models/notification.rb | 2 +- app/views/devise/menu/_login_items.html.erb | 4 ++-- ...20160108133501_add_notifications_counter_cache_to_user.rb | 5 +++++ db/schema.rb | 3 ++- 4 files changed, 10 insertions(+), 4 deletions(-) create mode 100644 db/migrate/20160108133501_add_notifications_counter_cache_to_user.rb diff --git a/app/models/notification.rb b/app/models/notification.rb index 7d3ebd146..1cb500ccf 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -1,5 +1,5 @@ class Notification < ActiveRecord::Base - belongs_to :user + belongs_to :user, counter_cache: true belongs_to :notifiable, polymorphic: true scope :unread, -> { all } diff --git a/app/views/devise/menu/_login_items.html.erb b/app/views/devise/menu/_login_items.html.erb index d2460d11d..7681619e1 100644 --- a/app/views/devise/menu/_login_items.html.erb +++ b/app/views/devise/menu/_login_items.html.erb @@ -2,9 +2,9 @@ <% if user_signed_in? %>
  • <%= link_to notifications_path, class: "notifications" do %> - <% if current_user.notifications.count > 0 %> + <% if current_user.notifications_count > 0 %> - + <% else %> diff --git a/db/migrate/20160108133501_add_notifications_counter_cache_to_user.rb b/db/migrate/20160108133501_add_notifications_counter_cache_to_user.rb new file mode 100644 index 000000000..e106df6ae --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 1ba98a0a3..162bd816e 100644 --- a/db/schema.rb +++ b/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: 20160108114750) do +ActiveRecord::Schema.define(version: 20160108133501) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -339,6 +339,7 @@ ActiveRecord::Schema.define(version: 20160108114750) 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 From 1c801a73989d77ad31cc85a2f6aaac234128ce91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juanjo=20Baz=C3=A1n?= Date: Fri, 8 Jan 2016 14:44:22 +0100 Subject: [PATCH 6/6] adds ignore_unused i18n keys --- config/i18n-tasks.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/i18n-tasks.yml b/config/i18n-tasks.yml index 081619394..6b815cd31 100644 --- a/config/i18n-tasks.yml +++ b/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}.*'