Merge branch 'stable' into develop

Changelog.md

@@ -90,6 +90,7 @@ Ruby 2.0 is no longer officially supported.
 * Fix broken default avatars in the database [#6014](https://github.com/diaspora/diaspora/pull/6014)
 * Only strip text direction codepoints around hashtags [#6067](https://github.com/diaspora/diaspora/issues/6067)
 * Fix selected week on admin weekly stats page [#6079](https://github.com/diaspora/diaspora/pull/6079)
+* Fix that some unread conversations may be hidden [#6060](https://github.com/diaspora/diaspora/pull/6060)
 
 ## Features
 * Hide post title of limited post in comment notification email [#5843](https://github.com/diaspora/diaspora/pull/5843)

app/controllers/conversations_controller.rb

@@ -5,35 +5,29 @@ class ConversationsController < ApplicationController
   respond_to :html, :mobile, :json, :js
 
   def index
-    @conversations = current_user.conversations.paginate(
-      :page => params[:page], :per_page => 15)
+    @visibilities = ConversationVisibility.includes(:conversation)
+                                          .order("conversations.updated_at DESC")
+                                          .where(person_id: current_user.person_id)
+                                          .paginate(page: params[:page], per_page: 15)
 
-    @visibilities = current_user.conversation_visibilities.paginate(
-      :page => params[:page], :per_page => 15)
+    if params[:conversation_id]
+      @conversation = Conversation.joins(:conversation_visibilities)
+                                  .where(conversation_visibilities: {
+                                           person_id:       current_user.person_id,
+                                           conversation_id: params[:conversation_id]
+                                         }).first
 
-    @conversation = Conversation.joins(:conversation_visibilities).where(
-      :conversation_visibilities => {:person_id => current_user.person_id, :conversation_id => params[:conversation_id]}).first
-
-    @unread_counts = {}
-    @visibilities.each { |v| @unread_counts[v.conversation_id] = v.unread }
-
-    @first_unread_message_id = @conversation.try(:first_unread_message, current_user).try(:id)
-
-    if @conversation
-      @conversation.set_read(current_user)
+      if @conversation
+        @first_unread_message_id = @conversation.first_unread_message(current_user).try(:id)
+        @conversation.set_read(current_user)
+      end
     end
 
-    @authors = {}
-    @conversations.each { |c| @authors[c.id] = c.last_author }
-
-    @ordered_participants = {}
-    @conversations.each { |c| @ordered_participants[c.id] = (c.messages.map{|m| m.author}.reverse + c.participants).uniq }
-
     gon.contacts = contacts_data
 
     respond_with do |format|
       format.html
-      format.json { render :json => @conversations, :status => 200 }
+      format.json { render json: @visibilities.map(&:conversation), status: 200 }
     end
   end
 

app/models/conversation.rb

@@ -72,11 +72,15 @@ class Conversation < ActiveRecord::Base
   end
 
   def last_author
-    return unless @last_author.present? || self.messages.size > 0
-    @last_author_id ||= self.messages.pluck(:author_id).last
+    return unless @last_author.present? || messages.size > 0
+    @last_author_id ||= messages.pluck(:author_id).last
     @last_author ||= Person.includes(:profile).where(id: @last_author_id).first
   end
 
+  def ordered_participants
+    @ordered_participants ||= (messages.map(&:author).reverse + participants).uniq
+  end
+
   def subject
     self[:subject].blank? ? "no subject" : self[:subject]
   end

app/models/user.rb

@@ -69,8 +69,8 @@ class User < ActiveRecord::Base
   has_many :blocks
   has_many :ignored_people, :through => :blocks, :source => :person
 
-  has_many :conversation_visibilities, -> { order 'updated_at DESC' }, through: :person
-  has_many :conversations, -> { order 'updated_at DESC' }, through: :conversation_visibilities
+  has_many :conversation_visibilities, through: :person
+  has_many :conversations, through: :conversation_visibilities
 
   has_many :notifications, :foreign_key => :recipient_id
 

app/views/conversations/_conversation.haml

@@ -2,11 +2,13 @@
 -#   licensed under the Affero General Public License version 3 or later.  See
 -#   the COPYRIGHT file.
 
+- conversation = visibility.conversation
 .conversation-wrapper{ :"data-conversation-path" => conversation_path(conversation) }
-  .stream_element.conversation{:data=>{:guid=>conversation.id}, :class => conversation_class(conversation, unread_counts[conversation.id].to_i, selected_conversation_id)}
+  .stream_element.conversation{data:  {guid: conversation.id},
+                               class: conversation_class(conversation, visibility.unread, @conversation.try(:id))}
     .media
       .img
-        - other_participants = ordered_participants[conversation.id] - [current_user.person]
+        - other_participants = conversation.ordered_participants - [current_user.person]
         - if other_participants.first.present?
           = person_image_tag(other_participants.first)
           - if other_participants.count > 1
@@ -16,18 +18,17 @@
       .bg
         .badge.badge-dafault.message_count
           = conversation.messages.size
-        - unread_count = unread_counts[conversation.id].to_i
-        - if unread_count > 0
+        - if visibility.unread > 0
           .badge.badge-important.unread_message_count
-            = unread_count
+            = visibility.unread
         .subject
           %div{ :class => direction_for(conversation.subject) }
             = conversation.subject
         .timestamp
           = timeago(conversation.updated_at)
         .last_author
-          - if authors[conversation.id].present?
-            = authors[conversation.id].name
+          - if conversation.last_author.present?
+            = conversation.last_author.name
         .last_message
           - if conversation.messages.present?
             %em

app/views/conversations/_conversation.mobile.haml

@@ -1,20 +1,20 @@
+- conversation = visibility.conversation
 %a.conversation{ :href => 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: ("unread" if visibility.unread > 0)}
     .media
       .img
         = person_image_tag(conversation.author, size: :thumb_small)
 
       .bg
-        = render(:partial => 'conversation_subject',
-                  :locals => { :conversation => conversation,
-                               :unread_count => unread_counts[conversation.id].to_i })
+        = render partial: "conversation_subject",
+                 locals:  { conversation: conversation, unread_count: visibility.unread }
 
         .last_author
           .timestamp
             = timeago(conversation.updated_at)
 
-          - if authors[conversation.id].present?
-            = authors[conversation.id].name
+          - if conversation.last_author.present?
+            = conversation.last_author.name
 
           - if conversation.participants.size > 2
             %span.participant_count

app/views/conversations/index.haml

@@ -10,22 +10,21 @@
       #left_pane
         #left_pane_header
           %h3
-            .pull-right{ class: ("hidden" unless @conversation)}
+            .pull-right{ class: ("hidden" unless @visibilities)}
               = link_to t('.new_conversation'), conversations_path, class: 'btn btn-default'
             = t('.inbox')
 
         #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, selected_conversation_id: @conversation.try(:id) }
+            - if @visibilities.count > 0
+              = render partial: "conversations/conversation", collection: @visibilities, as: :visibility
             - else
               #no_conversations
                 = t('.no_messages')
-            = will_paginate @conversations, previous_label: '«', next_label: '»',inner_window: 1,
+            = will_paginate @visibilities, previous_label: "«", next_label: "»",inner_window: 1,
                 renderer: WillPaginate::ActionView::BootstrapLinkRenderer
 
+
     .col-md-8
       - if @conversation
         .stream_container

app/views/conversations/index.mobile.haml

@@ -16,13 +16,12 @@
 
 #conversation_inbox
   .stream.conversations
-    - if @conversations.count > 0
-      = render partial: 'conversations/conversation', collection: @conversations,
-          locals: { authors: @authors,  unread_counts: @unread_counts }
+    - if @visibilities.count > 0
+      = render partial: "conversations/conversation", collection: @visibilities, as: :visibility
     - else
       .no-messages
         %i
           = t('.no_messages')
 
-  = will_paginate @conversations, previous_label: '«', next_label: '»', inner_window: 1, outer_window: 0,
+  = will_paginate @visibilities, previous_label: "«", next_label: "»", inner_window: 1, outer_window: 0,
       renderer: WillPaginate::ActionView::BootstrapLinkRenderer

spec/controllers/conversations_controller_spec.rb

@@ -58,38 +58,48 @@ describe ConversationsController, :type => :controller do
     end
   end
 
-  describe '#index' do
+  describe "#index" do
     before do
       hash = {
-        :author => alice.person,
-        :participant_ids => [alice.contacts.first.person.id, alice.person.id],
-        :subject => 'not spam',
-        :messages_attributes => [ {:author => alice.person, :text => 'cool stuff'} ]
+        author:              alice.person,
+        participant_ids:     [alice.contacts.first.person.id, alice.person.id],
+        subject:             "not spam",
+        messages_attributes: [{author: alice.person, text: "cool stuff"}]
       }
       @conversations = Array.new(3) { Conversation.create(hash) }
+      @visibilities = @conversations.map {|conversation|
+        conversation.conversation_visibilities.find {|visibility| visibility.person == alice.person }
+      }
     end
 
-    it 'succeeds' do
+    it "succeeds" do
       get :index
       expect(response).to be_success
-      expect(assigns[:conversations]).to match_array(@conversations)
+      expect(assigns[:visibilities]).to match_array(@visibilities)
     end
 
-    it 'succeeds with json' do
-      get :index, :format => :json
+    it "succeeds with json" do
+      get :index, format: :json
       expect(response).to be_success
       json = JSON.parse(response.body)
-      expect(json.first['conversation']).to be_present
+      expect(json.first["conversation"]).to be_present
     end
 
-    it 'retrieves all conversations for a user' do
+    it "retrieves all conversations for a user" do
       get :index
-      expect(assigns[:conversations].count).to eq(3)
+      expect(assigns[:visibilities].count).to eq(3)
     end
 
-    it 'does not let you access conversations where you are not a recipient' do
+    it "retrieves a conversation" do
+      get :index, conversation_id: @conversations.first.id
+      expect(response).to be_success
+      expect(assigns[:visibilities]).to match_array(@visibilities)
+      expect(assigns[:conversation]).to eq(@conversations.first)
+    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
+      get :index, conversation_id: @conversations.first.id
       expect(assigns[:conversation]).to be_nil
     end
   end

spec/javascripts/app/views/conversations_view_spec.js

@@ -3,6 +3,9 @@ describe("app.views.Conversations", function(){
     context("for unread conversations", function() {
       beforeEach(function() {
         spec.loadFixture("conversations_unread");
+        // select second conversation that is still unread
+        $(".conversation-wrapper > .conversation.selected").removeClass("selected")
+        $(".conversation-wrapper > .conversation.unread").addClass("selected")
       });
 
       it("removes the unread class from the conversation", function() {
@@ -21,16 +24,16 @@ describe("app.views.Conversations", 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");
+        expect($(".conversation-wrapper > .conversation .unread_message_count").text().trim()).toEqual("1");
         new app.views.Conversations();
-        expect($("#conversations_badge .badge_count").text().trim()).toEqual("1");
+        expect($("#conversations_badge .badge_count").text().trim()).toEqual("2");
       });
 
       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>";
+        var badge = "<div id=\"conversations_badge\"><div class=\"badge_count\">1</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");
+        expect($("#conversations_badge .badge_count").text().trim()).toEqual("1");
+        expect($(".conversation-wrapper > .conversation.selected .unread_message_count").text().trim()).toEqual("1");
         new app.views.Conversations();
         expect($("#conversations_badge .badge_count").text().trim()).toEqual("0");
         expect($("#conversations_badge .badge_count")).toHaveClass("hidden");

spec/models/conversation_spec.rb

@@ -32,6 +32,15 @@ describe Conversation, :type => :model do
     end
   end
 
+  describe "#ordered_participants" do
+    it "returns the ordered participants" do
+      cnv = Conversation.create(@create_hash)
+      Message.create(author: @user2.person, created_at: Time.now + 100, text: "last", conversation_id: cnv.id)
+      expect(cnv.ordered_participants.first).to eq(@user2.person)
+      expect(cnv.ordered_participants.last).to eq(@user1.person)
+    end
+  end
+
   describe '#first_unread_message' do
     before do
       @cnv = Conversation.create(@create_hash)