Conversations: fix badge count and automatic scrolling

app/assets/javascripts/app/views/conversations_view.js

@@ -6,18 +6,44 @@ app.views.Conversations = Backbone.View.extend({
 
   events: {
     "mouseenter .stream_element.conversation" : "showParticipants",
-    "mouseleave .stream_element.conversation" : "hideParticipants"
+    "mouseleave .stream_element.conversation" : "hideParticipants",
+    "conversation:loaded" : "setupConversation"
   },
 
   initialize: function() {
-    $("#people_stream.contacts .header .entypo").tooltip({ 'placement': 'bottom'});
-    // TODO doesn't work anymore
-    if ($('#first_unread').length > 0) {
-      $("html").scrollTop($('#first_unread').offset().top-50);
+    if($('#conversation_new:visible').length > 0) {
+      new app.views.ConversationsForm({contacts: gon.contacts});
     }
+    this.setupConversation();
+  },
 
-    new app.views.ConversationsForm({contacts: gon.contacts});
+  setupConversation: function() {
     app.helpers.timeago($(this.el));
+
+    var conv = $('.conversation-wrapper .stream_element.selected'),
+        cBadge = $('#conversations_badge .badge_count');
+
+    if(conv.hasClass('unread') ){
+      var unreadCount = parseInt(conv.find('.unread_message_count').text(), 10);
+
+      if(cBadge.text() !== '') {
+        cBadge.text().replace(/\d+/, function(num){
+          num = parseInt(num, 10) - unreadCount;
+          if(num > 0) {
+            cBadge.text(num);
+          } else {
+            cBadge.text(0).addClass('hidden');
+          }
+        });
+      }
+      conv.removeClass('unread');
+      conv.find('.unread_message_count').remove();
+
+      var pos = $('#first_unread').offset().top - 50;
+      $("html").animate({scrollTop:pos});
+    } else {
+      $("html").animate({scrollTop:0});
+    }
   },
 
   hideParticipants: function(e){

app/assets/javascripts/inbox.js

@@ -1,41 +1,17 @@
 // @license magnet:?xt=urn:btih:0b31508aeb0634b347b8270c7bee4d411b5d4109&dn=agpl-3.0.txt AGPL-v3-or-Later
-
-/*   Copyright (c) 2010-2011, Diaspora Inc.  This file is
- *   licensed under the Affero General Public License version 3 or later.  See
- *   the COPYRIGHT file.
- */
-
 $(document).ready(function(){
   $(document).on('click', '.conversation-wrapper', function(){
     var conversation_path = $(this).data('conversation-path');
-
     $.getScript(conversation_path, function() {
       Diaspora.page.directionDetector.updateBinds();
     });
-
     history.pushState(null, "", conversation_path);
-
-    var conv = $(this).children('.stream_element'),
-        cBadge = $("#conversations_badge .badge_count");
-    if(conv.hasClass('unread') ){
-      conv.removeClass('unread');
-    }
-    if(cBadge.html() !== null) {
-      cBadge.html().replace(/\d+/, function(num){
-        num = parseInt(num);
-        cBadge.html(parseInt(num)-1);
-        if(num === 1) {
-          cBadge.addClass("hidden");
-        }
-      });
-    }
-
     return false;
   });
 
   $(window).bind("popstate", function(){
     if (location.href.match(/conversations\/\d+/) !== null) {
-	  $.getScript(location.href, function() {
+      $.getScript(location.href, function() {
         Diaspora.page.directionDetector.updateBinds();
       });
       return false;

app/controllers/conversations_controller.rb

@@ -21,6 +21,10 @@ class ConversationsController < ApplicationController
 
     @first_unread_message_id = @conversation.try(:first_unread_message, current_user).try(:id)
 
+    if @conversation
+      @conversation.set_read(current_user)
+    end
+
     @authors = {}
     @conversations.each { |c| @authors[c.id] = c.last_author }
 
@@ -65,21 +69,21 @@ class ConversationsController < ApplicationController
   end
 
   def show
-    if @conversation = current_user.conversations.where(id: params[:id]).first
-
-      @first_unread_message_id = @conversation.first_unread_message(current_user).try(:id)
-      if @visibility = ConversationVisibility.where(:conversation_id => params[:id], :person_id => current_user.person.id).first
-        @visibility.unread = 0
-        @visibility.save
+    respond_to do |format|
+      format.html do
+        redirect_to conversations_path(:conversation_id => params[:id])
+        return
       end
 
-      respond_to do |format|
-        format.html { redirect_to conversations_path(:conversation_id => @conversation.id) }
+      if @conversation = current_user.conversations.where(id: params[:id]).first
+        @first_unread_message_id = @conversation.first_unread_message(current_user).try(:id)
+        @conversation.set_read(current_user)
+
         format.js
         format.json { render :json => @conversation, :status => 200 }
+      else
+        redirect_to conversations_path
       end
-    else
-      redirect_to conversations_path
     end
   end
 

app/helpers/conversations_helper.rb

@@ -0,0 +1,9 @@
+module ConversationsHelper
+  def conversation_class(conversation, unread_count, selected_conversation_id)
+    conv_class = unread_count > 0 ? "unread " : ""
+    if selected_conversation_id && conversation.id == selected_conversation_id
+      conv_class << "selected"
+    end
+    conv_class
+  end
+end

app/models/conversation.rb

@@ -51,6 +51,13 @@ class Conversation < ActiveRecord::Base
     end
   end
 
+  def set_read(user)
+    if visibility = self.conversation_visibilities.where(:person_id => user.person.id).first
+      visibility.unread = 0
+      visibility.save
+    end
+  end
+
   def public?
     false
   end

app/views/conversations/_conversation.haml

@@ -3,7 +3,7 @@
 -#   the COPYRIGHT file.
 
 .conversation-wrapper{ :"data-conversation-path" => conversation_path(conversation) }
-  .stream_element.conversation{:data=>{:guid=>conversation.id}, :class => ('unread' if unread_counts[conversation.id].to_i > 0)}
+  .stream_element.conversation{:data=>{:guid=>conversation.id}, :class => conversation_class(conversation, unread_counts[conversation.id].to_i, selected_conversation_id)}
     .media
       .img
         - other_participants = ordered_participants[conversation.id] - [current_user.person]

app/views/conversations/index.haml

@@ -17,7 +17,7 @@
         #conversation_inbox
           .stream.conversations
             - if @conversations.count > 0
-              = render :partial => 'conversations/conversation', :collection => @conversations, :locals => {:authors => @authors, :ordered_participants => @ordered_participants, :unread_counts => @unread_counts}
+              = render :partial => 'conversations/conversation', :collection => @conversations, :locals => {:authors => @authors, :ordered_participants => @ordered_participants, :unread_counts => @unread_counts, :selected_conversation_id => @conversation.try(:id)}
             - else
               #no_conversations
                 = t('.no_messages')

app/views/conversations/show.js.erb

@@ -8,10 +8,4 @@ $('#conversation_show').html("<%= escape_javascript(render('conversations/show',
 
 $(".stream_element", "#conversation_inbox").removeClass('selected');
 $(".stream_element[data-guid='<%= @conversation.id %>']", "#conversation_inbox").addClass('selected');
-$(".stream_element[data-guid='<%= @conversation.id %>']", "#conversation_inbox").find(".unread_message_count").remove()
-
-app.helpers.timeago($(document));
-
-if ($('#first_unread') > 0) {
-  $("html").scrollTop($('#first_unread').offset().top-50);
-}
+$('#conversation_show').trigger("conversation:loaded");

spec/controllers/conversations_controller_spec.rb

@@ -86,6 +86,12 @@ describe ConversationsController, :type => :controller do
       get :index
       expect(assigns[:conversations].count).to eq(3)
     end
+
+    it 'does not let you access conversations where you are not a recipient' do
+      sign_in :user, eve
+      get :index, :conversation_id => @conversations.first.id
+      expect(assigns[:conversation]).to be_nil
+    end
   end
 
   describe '#create' do
@@ -291,14 +297,6 @@ describe ConversationsController, :type => :controller do
     it 'redirects to index' do
       get :show, :id => @conversation.id
       expect(response).to redirect_to(conversations_path(:conversation_id => @conversation.id))
-      expect(assigns[:conversation]).to eq(@conversation)
-    end
-
-    it 'does not let you access conversations where you are not a recipient' do
-      sign_in :user, eve
-
-      get :show, :id => @conversation.id
-      expect(response.code).to redirect_to conversations_path
     end
   end
 end

spec/controllers/jasmine_fixtures/conversations_spec.rb

@@ -0,0 +1,34 @@
+require 'spec_helper'
+
+describe ConversationsController, :type => :controller do
+  describe '#index' do
+    before do
+      @person = alice.contacts.first.person
+      hash = {
+        :author => @person,
+        :participant_ids => [alice.person.id, @person.id],
+        :subject => 'not spam',
+        :messages_attributes => [ {:author => @person, :text => 'cool stuff'} ]
+      }
+      @conv1 = Conversation.create(hash)
+      Message.create(:author => @person, :created_at => Time.now + 100, :text => "message", :conversation_id => @conv1.id)
+             .increase_unread(alice)
+      Message.create(:author => @person, :created_at => Time.now + 200, :text => "another message", :conversation_id => @conv1.id)
+             .increase_unread(alice)
+
+      @conv2 = Conversation.create(hash)
+      Message.create(:author => @person, :created_at => Time.now + 100, :text => "message", :conversation_id => @conv2.id)
+             .increase_unread(alice)
+
+      sign_in :user, alice
+    end
+
+    it "generates a jasmine fixture", :fixture => true do
+      get :index, :conversation_id => @conv1.id
+      save_fixture(html_for("body"), "conversations_unread")
+
+      get :index, :conversation_id => @conv1.id
+      save_fixture(html_for("body"), "conversations_read")
+    end
+  end
+end

spec/helpers/conversations_helper_spec.rb

@@ -0,0 +1,34 @@
+require 'spec_helper'
+
+describe ConversationsHelper, :type => :helper do
+  before do
+    @conversation = FactoryGirl.create(:conversation)
+  end
+
+  describe '#conversation_class' do
+    it 'returns an empty string as default' do
+      expect(conversation_class(@conversation, 0, nil)).to eq('')
+      expect(conversation_class(@conversation, 0, @conversation.id+1)).to eq('')
+    end
+
+    it 'includes unread for unread conversations' do
+      expect(conversation_class(@conversation, 1, nil)).to include('unread')
+      expect(conversation_class(@conversation, 42, @conversation.id+1)).to include('unread')
+      expect(conversation_class(@conversation, 42, @conversation.id)).to include('unread')
+    end
+
+    it 'does not include unread for read conversations' do
+      expect(conversation_class(@conversation, 0, @conversation.id)).to_not include('unread')
+    end
+
+    it 'includes selected for selected conversations' do
+      expect(conversation_class(@conversation, 0, @conversation.id)).to include('selected')
+      expect(conversation_class(@conversation, 1, @conversation.id)).to include('selected')
+    end
+
+    it 'does not include selected for not selected conversations' do
+      expect(conversation_class(@conversation, 1, @conversation.id+1)).to_not include('selected')
+      expect(conversation_class(@conversation, 1, nil)).to_not include('selected')
+    end
+  end
+end

spec/javascripts/app/views/conversations_view_spec.js

@@ -0,0 +1,54 @@
+describe("app.views.Conversations", function(){
+  describe('setupConversation', function() {
+    context('for unread conversations', function() {
+      beforeEach(function() {
+        spec.loadFixture('conversations_unread');
+      });
+
+      it('removes the unread class from the conversation', function() {
+        expect($('.conversation-wrapper > .conversation.selected')).toHaveClass('unread');
+        new app.views.Conversations();
+        expect($('.conversation-wrapper > .conversation.selected')).not.toHaveClass('unread');
+      });
+
+      it('removes the unread message counter from the conversation', function() {
+        expect($('.conversation-wrapper > .conversation.selected .unread_message_count').length).toEqual(1);
+        new app.views.Conversations();
+        expect($('.conversation-wrapper > .conversation.selected .unread_message_count').length).toEqual(0);
+      });
+
+      it('decreases the unread message count in the header', function() {
+        var badge = '<div id="conversations_badge"><div class="badge_count">3</div></div>';
+        $('header').append(badge);
+        expect($('#conversations_badge .badge_count').text().trim()).toEqual('3');
+        expect($('.conversation-wrapper > .conversation.selected .unread_message_count').text().trim()).toEqual('2');
+        new app.views.Conversations();
+        expect($('#conversations_badge .badge_count').text().trim()).toEqual('1');
+      });
+
+      it('removes the badge_count in the header if there are no unread messages left', function() {
+        var badge = '<div id="conversations_badge"><div class="badge_count">2</div></div>';
+        $('header').append(badge);
+        expect($('#conversations_badge .badge_count').text().trim()).toEqual('2');
+        expect($('.conversation-wrapper > .conversation.selected .unread_message_count').text().trim()).toEqual('2');
+        new app.views.Conversations();
+        expect($('#conversations_badge .badge_count').text().trim()).toEqual('0');
+        expect($('#conversations_badge .badge_count')).toHaveClass('hidden');
+      });
+    });
+
+    context('for read conversations', function() {
+      beforeEach(function() {
+        spec.loadFixture('conversations_read');
+      });
+
+      it('does not change the badge_count in the header', function() {
+        var badge = '<div id="conversations_badge"><div class="badge_count">3</div></div>';
+        $('header').append(badge);
+        expect($('#conversations_badge .badge_count').text().trim()).toEqual('3');
+        new app.views.Conversations();
+        expect($('#conversations_badge .badge_count').text().trim()).toEqual('3');
+      });
+    });
+  });
+});

spec/models/conversation_spec.rb

@@ -32,16 +32,16 @@ describe Conversation, :type => :model do
     end
   end
 
-  describe '#first_unread_message' do  
+  describe '#first_unread_message' do
     before do
       @cnv = Conversation.create(@create_hash)
       @message = Message.create(:author => @user2.person, :created_at => Time.now + 100, :text => "last", :conversation_id => @cnv.id)
-      @message.increase_unread(@user1) 
+      @message.increase_unread(@user1)
     end
-    
+
     it 'returns the first unread message if there are unread messages in a conversation' do
       @cnv.first_unread_message(@user1) == @message
-    end  
+    end
 
     it 'returns nil if there are no unread messages in a conversation' do
       @cnv.conversation_visibilities.where(:person_id => @user1.person.id).first.tap { |cv| cv.unread = 0 }.save
@@ -49,6 +49,22 @@ describe Conversation, :type => :model do
     end
   end
 
+  describe '#set_read' do
+    before do
+      @cnv = Conversation.create(@create_hash)
+      Message.create(:author => @user2.person, :created_at => Time.now + 100, :text => "first", :conversation_id => @cnv.id)
+             .increase_unread(@user1)
+      Message.create(:author => @user2.person, :created_at => Time.now + 200, :text => "last", :conversation_id => @cnv.id)
+             .increase_unread(@user1)
+    end
+
+    it 'sets the unread counter to 0' do
+      expect(@cnv.conversation_visibilities.where(:person_id => @user1.person.id).first.unread).to eq(2)
+      @cnv.set_read(@user1)
+      expect(@cnv.conversation_visibilities.where(:person_id => @user1.person.id).first.unread).to eq(0)
+    end
+  end
+
   context 'transport' do
     before do
       @cnv = Conversation.create(@create_hash)
@@ -118,7 +134,7 @@ describe Conversation, :type => :model do
         :messages_attributes => [ {:author => peter.person, :text => 'hey'} ]
       }
     end
-    
+
     it 'with invalid recipient' do
       conversation = Conversation.create(@invalid_hash)
       expect(conversation).to be_invalid