From f2be6e8bcfa2b0abd7a0f2b0029d2455ef8c3860 Mon Sep 17 00:00:00 2001 From: Steven Fuchs Date: Thu, 22 Dec 2011 15:20:48 -0500 Subject: [PATCH 01/13] controller can accept a parameter as to whether the notification should be marked as read or unread --- app/controllers/notifications_controller.rb | 2 +- spec/controllers/notifications_controller_spec.rb | 12 +++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index c27df96c7..22ac5fa5f 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -11,7 +11,7 @@ class NotificationsController < VannaController def update(opts=params) note = Notification.where(:recipient_id => current_user.id, :id => opts[:id]).first if note - note.update_attributes(:unread => false) + note.update_attributes(:unread => opts[:is_unread] || false ) {} else Response.new :status => 404 diff --git a/spec/controllers/notifications_controller_spec.rb b/spec/controllers/notifications_controller_spec.rb index 3f1e64b1f..baf59f870 100644 --- a/spec/controllers/notifications_controller_spec.rb +++ b/spec/controllers/notifications_controller_spec.rb @@ -14,11 +14,21 @@ describe NotificationsController do end describe '#update' do - it 'marks a notification as read' do + it 'marks a notification as read if it gets no other information' do note = Factory(:notification, :recipient => @user) @controller.update :id => note.id Notification.first.unread.should == false end + it 'marks a notification as read if it is told to' do + note = Factory(:notification, :recipient => @user) + @controller.update :id => note.id, :is_unread => false + Notification.first.unread.should == false + end + it 'marks a notification as unread if it is told to' do + note = Factory(:notification, :recipient => @user) + @controller.update :id => note.id, :is_unread => true + Notification.first.unread.should == true + end it 'only lets you read your own notifications' do user2 = bob From a7af886da2340c490aecec6605717eaeb5a12116 Mon Sep 17 00:00:00 2001 From: Steven Fuchs Date: Thu, 22 Dec 2011 15:53:14 -0500 Subject: [PATCH 02/13] add 'read' class ro read messages to match the 'unread' class associated with unread messages. This will keep us from attaching jquery functionality to notification types we may not know about. --- app/views/notifications/index.html.haml | 2 +- app/views/notifications/index.mobile.haml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/notifications/index.html.haml b/app/views/notifications/index.html.haml index 88a098ab2..0935dea0e 100644 --- a/app/views/notifications/index.html.haml +++ b/app/views/notifications/index.html.haml @@ -21,7 +21,7 @@ .span-8.notifications_for_day - notes.each do |note| - .stream_element{:data=>{:guid => note.id}, :class => "#{note.unread ? 'unread' : ''}"} + .stream_element{:data=>{:guid => note.id}, :class => "#{note.unread ? 'unread' : 'read'}"} - if note.type == "Notifications::StartedSharing" && contact = current_user.contact_for(note[:target]) .right = aspect_membership_dropdown(contact, note[:target], 'left') diff --git a/app/views/notifications/index.mobile.haml b/app/views/notifications/index.mobile.haml index 18ea33fef..3f27eb8bc 100644 --- a/app/views/notifications/index.mobile.haml +++ b/app/views/notifications/index.mobile.haml @@ -8,7 +8,7 @@ = day %ul.notifications_for_day - notes.each do |note| - .stream_element{:data=>{:guid => note.id}, :class => "#{note.unread ? 'unread' : ''}"} + .stream_element{:data=>{:guid => note.id}, :class => "#{note.unread ? 'unread' : 'read'}"} = person_image_link(note.actors.last) From b6dd14a968b344217ceb446284d006c73ff0be6d Mon Sep 17 00:00:00 2001 From: Steven Fuchs Date: Thu, 22 Dec 2011 15:56:03 -0500 Subject: [PATCH 03/13] clicking on notifications takes note of the current UI state and makes sure that the resulting 'read' state matches the users expectation. --- public/javascripts/widgets/notifications.js | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/public/javascripts/widgets/notifications.js b/public/javascripts/widgets/notifications.js index fff80b404..f4a873a82 100644 --- a/public/javascripts/widgets/notifications.js +++ b/public/javascripts/widgets/notifications.js @@ -15,11 +15,18 @@ notificationArea: notificationArea }); - $(".stream_element.unread").live("mousedown", function() { - self.decrementCount(); - + $(".stream_element.unread,.stream_element.read").live("mousedown", function() { + if ( $(this).hasClass("unread") ) { + self.decrementCount(); + $(this).removeClass("unread").addClass( "read" ) + } + else { + self.incrementCount(); + $(this).removeClass("read").addClass( "unread" ) + } $.ajax({ - url: "notifications/" + $(this).removeClass("unread").data("guid"), + url: "notifications/" + $(this).data("guid"), + data: { is_unread: $(this).hasClass("unread") }, type: "PUT" }); }); From 4d6f53401b6a83c192d4849ea6b241decec8ac45 Mon Sep 17 00:00:00 2001 From: Steven Fuchs Date: Thu, 22 Dec 2011 15:57:59 -0500 Subject: [PATCH 04/13] makes sure that the large notification count badge on the notifications page gets incremented and decremented. Not the neatest solution since this code can be called from many pages, but shouldn't cause any problems in that case. --- public/javascripts/widgets/notifications.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/public/javascripts/widgets/notifications.js b/public/javascripts/widgets/notifications.js index f4a873a82..1f0eec25a 100644 --- a/public/javascripts/widgets/notifications.js +++ b/public/javascripts/widgets/notifications.js @@ -57,12 +57,15 @@ if(self.badge.text() !== "") { self.badge.text(self.count); + $( ".notification_count" ).text(self.count); if(self.count === 0) { self.badge.addClass("hidden"); + $( ".notification_count" ).addClass("hidden"); } else if(self.count === 1) { self.badge.removeClass("hidden"); + $( ".notification_count" ).removeClass("hidden"); } } }; From f3486e831fd55f991dd967a167b5018959c2cf0e Mon Sep 17 00:00:00 2001 From: Steven Fuchs Date: Thu, 22 Dec 2011 22:57:05 -0500 Subject: [PATCH 05/13] changes is_unread parameter to unread --- app/controllers/notifications_controller.rb | 2 +- spec/controllers/notifications_controller_spec.rb | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index 22ac5fa5f..07853afed 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -11,7 +11,7 @@ class NotificationsController < VannaController def update(opts=params) note = Notification.where(:recipient_id => current_user.id, :id => opts[:id]).first if note - note.update_attributes(:unread => opts[:is_unread] || false ) + note.update_attributes(:unread => opts[:unread] || false ) {} else Response.new :status => 404 diff --git a/spec/controllers/notifications_controller_spec.rb b/spec/controllers/notifications_controller_spec.rb index baf59f870..389cdca26 100644 --- a/spec/controllers/notifications_controller_spec.rb +++ b/spec/controllers/notifications_controller_spec.rb @@ -21,12 +21,16 @@ describe NotificationsController do end it 'marks a notification as read if it is told to' do note = Factory(:notification, :recipient => @user) - @controller.update :id => note.id, :is_unread => false + @controller.update :id => note.id, :unread => false + Notification.first.unread.should == false + @controller.update :id => note.id, :unread => "false" Notification.first.unread.should == false end it 'marks a notification as unread if it is told to' do note = Factory(:notification, :recipient => @user) - @controller.update :id => note.id, :is_unread => true + @controller.update :id => note.id, :unread => "true" + Notification.first.unread.should == true + @controller.update :id => note.id, :unread => true Notification.first.unread.should == true end From 1519cdd672bc7578cafbe66366c9722075bedd8d Mon Sep 17 00:00:00 2001 From: Steven Fuchs Date: Thu, 22 Dec 2011 22:57:33 -0500 Subject: [PATCH 06/13] new parameter name and fix to notification badge css --- public/javascripts/widgets/notifications.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/public/javascripts/widgets/notifications.js b/public/javascripts/widgets/notifications.js index 1f0eec25a..e42bad51d 100644 --- a/public/javascripts/widgets/notifications.js +++ b/public/javascripts/widgets/notifications.js @@ -26,7 +26,7 @@ } $.ajax({ url: "notifications/" + $(this).data("guid"), - data: { is_unread: $(this).hasClass("unread") }, + data: { unread: $(this).hasClass("unread") }, type: "PUT" }); }); @@ -61,11 +61,11 @@ if(self.count === 0) { self.badge.addClass("hidden"); - $( ".notification_count" ).addClass("hidden"); + $( ".notification_count" ).removeClass("unread"); } else if(self.count === 1) { self.badge.removeClass("hidden"); - $( ".notification_count" ).removeClass("hidden"); + $( ".notification_count" ).addClass("unread"); } } }; From cd75c6ea67a4ec16e22b303f641d306c37c90d7e Mon Sep 17 00:00:00 2001 From: Steven Fuchs Date: Fri, 23 Dec 2011 11:36:20 -0500 Subject: [PATCH 07/13] cleaner reading of unread parameter --- 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 07853afed..c2e0e5a79 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -11,7 +11,7 @@ class NotificationsController < VannaController def update(opts=params) note = Notification.where(:recipient_id => current_user.id, :id => opts[:id]).first if note - note.update_attributes(:unread => opts[:unread] || false ) + note.update_attributes(:unread => opts[:unread].to_s == "true" ) {} else Response.new :status => 404 From a1d474111b0929ff7223ddfde62c4b19dd474519 Mon Sep 17 00:00:00 2001 From: Steven Fuchs Date: Fri, 23 Dec 2011 15:35:07 -0500 Subject: [PATCH 08/13] controller now only accepts string values for unread. --- app/controllers/notifications_controller.rb | 2 +- spec/controllers/notifications_controller_spec.rb | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index c2e0e5a79..28fb48922 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -11,7 +11,7 @@ class NotificationsController < VannaController def update(opts=params) note = Notification.where(:recipient_id => current_user.id, :id => opts[:id]).first if note - note.update_attributes(:unread => opts[:unread].to_s == "true" ) + note.update_attributes(:unread => opts[:unread] == "true" ) {} else Response.new :status => 404 diff --git a/spec/controllers/notifications_controller_spec.rb b/spec/controllers/notifications_controller_spec.rb index 389cdca26..e404334fa 100644 --- a/spec/controllers/notifications_controller_spec.rb +++ b/spec/controllers/notifications_controller_spec.rb @@ -21,8 +21,6 @@ describe NotificationsController do end it 'marks a notification as read if it is told to' do note = Factory(:notification, :recipient => @user) - @controller.update :id => note.id, :unread => false - Notification.first.unread.should == false @controller.update :id => note.id, :unread => "false" Notification.first.unread.should == false end @@ -30,8 +28,6 @@ describe NotificationsController do note = Factory(:notification, :recipient => @user) @controller.update :id => note.id, :unread => "true" Notification.first.unread.should == true - @controller.update :id => note.id, :unread => true - Notification.first.unread.should == true end it 'only lets you read your own notifications' do From f72a4b4476d2845348f2b021779ed2f042188ac8 Mon Sep 17 00:00:00 2001 From: Steven Fuchs Date: Fri, 23 Dec 2011 22:58:01 -0500 Subject: [PATCH 09/13] notifications controller should return the guid and unread state of the updated notification --- app/controllers/notifications_controller.rb | 2 +- spec/controllers/notifications_controller_spec.rb | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index 28fb48922..deeb59e1b 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -12,7 +12,7 @@ class NotificationsController < VannaController note = Notification.where(:recipient_id => current_user.id, :id => opts[:id]).first if note note.update_attributes(:unread => opts[:unread] == "true" ) - {} + { :guid => note.id, :unread => note.unread } else Response.new :status => 404 end diff --git a/spec/controllers/notifications_controller_spec.rb b/spec/controllers/notifications_controller_spec.rb index e404334fa..d7dce0014 100644 --- a/spec/controllers/notifications_controller_spec.rb +++ b/spec/controllers/notifications_controller_spec.rb @@ -30,6 +30,17 @@ describe NotificationsController do Notification.first.unread.should == true end + it 'should return the item guid' do + note = Factory(:notification, :recipient => @user) + answer = @controller.update :id => note.id + answer[:guid].should == note.id + end + it 'should return the unread state' do + note = Factory(:notification, :recipient => @user) + answer = @controller.update :id => note.id, :unread => "true" + answer[:unread].should == true + end + it 'only lets you read your own notifications' do user2 = bob From f23ac92b1e8c86ce52de7292680ca20ff3b98818 Mon Sep 17 00:00:00 2001 From: Steven Fuchs Date: Fri, 23 Dec 2011 22:58:38 -0500 Subject: [PATCH 10/13] notification elements should have a hover state since they are live. --- public/stylesheets/sass/application.sass | 2 ++ 1 file changed, 2 insertions(+) diff --git a/public/stylesheets/sass/application.sass b/public/stylesheets/sass/application.sass index 0b9116ae2..a17ef2aad 100644 --- a/public/stylesheets/sass/application.sass +++ b/public/stylesheets/sass/application.sass @@ -3130,6 +3130,8 @@ a.toggle_selector .notification_element :padding 10px :min-height 30px + &:hover + :background-color #FAFAFA > img :height 30px From 02531337cba38ff1ce9615a1318623df4dfc5382 Mon Sep 17 00:00:00 2001 From: Steven Fuchs Date: Fri, 23 Dec 2011 23:00:16 -0500 Subject: [PATCH 11/13] break actions into separate functions. Make css changes happen inside the completion functions --- public/javascripts/widgets/notifications.js | 44 +++++++++++++-------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/public/javascripts/widgets/notifications.js b/public/javascripts/widgets/notifications.js index e42bad51d..e16136d2a 100644 --- a/public/javascripts/widgets/notifications.js +++ b/public/javascripts/widgets/notifications.js @@ -15,21 +15,7 @@ notificationArea: notificationArea }); - $(".stream_element.unread,.stream_element.read").live("mousedown", function() { - if ( $(this).hasClass("unread") ) { - self.decrementCount(); - $(this).removeClass("unread").addClass( "read" ) - } - else { - self.incrementCount(); - $(this).removeClass("read").addClass( "unread" ) - } - $.ajax({ - url: "notifications/" + $(this).data("guid"), - data: { unread: $(this).hasClass("unread") }, - type: "PUT" - }); - }); + $(".stream_element.unread,.stream_element.read").live("mousedown", self.messageClick); $("a.more").live("click", function(evt) { evt.preventDefault(); @@ -38,7 +24,33 @@ .removeClass("hidden"); }); }); - + this.messageClick = function() { + $.ajax({ + url: "notifications/" + $(this).data("guid"), + data: { unread: $(this).hasClass("read") }, + type: "PUT", + success: self.clickSuccess + }); + }; + this.clickSuccess = function( data ) { + var jsList = jQuery.parseJSON(data); + var itemID = jsList["guid"] + var isUnread = jsList["unread"] + if ( isUnread ) { + self.incrementCount(); + }else{ + self.decrementCount(); + } + $('.read,.unread').each(function(index) { + if ( $(this).data("guid") == itemID ) { + if ( isUnread ) { + $(this).removeClass("read").addClass( "unread" ) + } else { + $(this).removeClass("unread").addClass( "read" ) + } + } + }); + }; this.showNotification = function(notification) { $(notification.html).prependTo(this.notificationArea) .fadeIn(200) From 95b553f95386b451ff109953059a29f4302a1a43 Mon Sep 17 00:00:00 2001 From: Steven Fuchs Date: Fri, 23 Dec 2011 23:01:18 -0500 Subject: [PATCH 12/13] notifications popup menu should use functions from regular notification object. --- public/javascripts/widgets/notifications-badge.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/public/javascripts/widgets/notifications-badge.js b/public/javascripts/widgets/notifications-badge.js index e64d5da39..f8499ffce 100644 --- a/public/javascripts/widgets/notifications-badge.js +++ b/public/javascripts/widgets/notifications-badge.js @@ -63,6 +63,7 @@ $.each(notifications, function(index, notification) { var notificationElement = $("
") .addClass("notification_element") + .data( "guid", notification.id ) .html(notification.translation) .prepend($("", { src: notification.actors[0].avatar })) .append("
") @@ -73,16 +74,18 @@ .appendTo(self.dropdownNotifications); notificationElement.find("abbr.timeago").timeago(); + notificationElement.click(Diaspora.page.header.notifications.messageClick); if(notification.unread) { notificationElement.addClass("unread"); $.ajax({ url: "/notifications/" + notification.id, type: "PUT", - success: function() { - Diaspora.page.header.notifications.decrementCount(); - } + data: { unread: false }, + success: Diaspora.page.header.notifications.clickSuccess }); + } else { + notificationElement.addClass("read"); } }); }); From 176797628e9a4fffc79b73759433a4807836c0cc Mon Sep 17 00:00:00 2001 From: Steven Fuchs Date: Tue, 10 Jan 2012 00:07:42 -0500 Subject: [PATCH 13/13] javascript tests for the notification click callback routine. --- .../javascripts/widgets/notifications-spec.js | 33 ++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/spec/javascripts/widgets/notifications-spec.js b/spec/javascripts/widgets/notifications-spec.js index cbb962180..f9ae7ee80 100644 --- a/spec/javascripts/widgets/notifications-spec.js +++ b/spec/javascripts/widgets/notifications-spec.js @@ -3,13 +3,44 @@ * the COPYRIGHT file. */ describe("Diaspora.Widgets.Notifications", function() { - var changeNotificationCountSpy, notifications; + var changeNotificationCountSpy, notifications, incrementCountSpy, decrementCountSpy; beforeEach(function() { spec.loadFixture("aspects_index"); notifications = Diaspora.BaseWidget.instantiate("Notifications", $("#notifications"), $("#notification_badge .badge_count")); changeNotificationCountSpy = spyOn(notifications, "changeNotificationCount").andCallThrough(); + incrementCountSpy = spyOn(notifications, "incrementCount").andCallThrough(); + decrementCountSpy = spyOn(notifications, "decrementCount").andCallThrough(); + }); + + describe("clickSuccess", function(){ + it("changes the css to a read cell", function() { + $(".notifications").html( + '
' + + '
' + ); + notifications.clickSuccess(JSON.stringify({guid:2,unread:false})); + expect( $('.stream_element#2')).toHaveClass("read"); + }); + it("changes the css to an unread cell", function() { + $(".notifications").html( + '
' + + '
' + ); + notifications.clickSuccess(JSON.stringify({guid:1,unread:true})); + expect( $('.stream_element#1')).toHaveClass("unread"); + }); + + + it("calls Notifications.decrementCount on a read cell", function() { + notifications.clickSuccess(JSON.stringify({guid:1,unread:false})); + expect(notifications.decrementCount).toHaveBeenCalled(); + }); + it("calls Notifications.incrementCount on a unread cell", function() { + notifications.clickSuccess(JSON.stringify({guid:1,unread:true})); + expect(notifications.incrementCount).toHaveBeenCalled(); + }); }); describe("decrementCount", function() {