refactor: iterate over visiblities

closes #6060

Changelog.md

@@ -57,6 +57,7 @@
 * 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

@@ -23,23 +23,11 @@ class ConversationsController < ApplicationController
       end
     end
 
-    @conversations = []
-    @unread_counts = {}
-    @authors = {}
-    @ordered_participants = {}
-    @visibilities.each {|v|
-      @unread_counts[v.conversation_id] = v.unread
-      c = v.conversation
-      @conversations << c
-      @authors[c.id] = c.last_author
-      @ordered_participants[c.id] = (c.messages.map(&: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
+    return unless @last_author.present? || messages.size > 0
-    @last_author_id ||= self.messages.pluck(:author_id).last
+    @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/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 visibility.unread > 0
-        - if unread_count > 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?
+          - if conversation.last_author.present?
-            = authors[conversation.id].name
+            = 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)
 
       .bg
-        = render(:partial => 'conversation_subject',
+        = render partial: "conversation_subject",
-                  :locals => { :conversation => conversation,
+                 locals:  { conversation: conversation, unread_count: visibility.unread }
-                               :unread_count => unread_counts[conversation.id].to_i })
 
         .last_author
           .timestamp
             = timeago(conversation.updated_at)
 
-          - if authors[conversation.id].present?
+          - if conversation.last_author.present?
-            = authors[conversation.id].name
+            = conversation.last_author.name
 
           - if conversation.participants.size > 2
             %span.participant_count

app/views/conversations/index.haml

@@ -10,18 +10,19 @@
       #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'
+              = link_to t('.new_conversation'), conversations_path, class: 'btn btn-default'
             = t('.inbox')
 
         #conversation_inbox
           .stream.conversations
-            - if @conversations.count > 0
+            - if @visibilities.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)}
+              = render partial: "conversations/conversation", collection: @visibilities, as: :visibility, locals: {authors: @authors, ordered_participants: @ordered_participants, unread_counts: @unread_counts, selected_conversation_id: @conversation.try(:id)}
             - else
               #no_conversations
                 = t('.no_messages')
-            = will_paginate @visibilities, :previous_label => "«", :next_label => "»", :inner_window => 1, :renderer => WillPaginate::ActionView::BootstrapLinkRenderer
+            = will_paginate @visibilities, previous_label: "«", next_label: "»",inner_window: 1,
+                renderer: WillPaginate::ActionView::BootstrapLinkRenderer
 
     .span8
       - if @conversation

app/views/conversations/index.mobile.haml

@@ -16,8 +16,8 @@
 
 #conversation_inbox
   .stream.conversations
-    - if @conversations.count > 0
+    - if @visibilities.count > 0
-      = render :partial => 'conversations/conversation', :collection => @conversations, :locals => {:authors => @authors, :unread_counts => @unread_counts}
+      = render partial: "conversations/conversation", collection: @visibilities, as: :visibility, locals: {authors: @authors, unread_counts: @unread_counts}
     - else
       %br
       %br
@@ -27,4 +27,5 @@
         %i
           = t('.no_messages')
 
-  = will_paginate @conversations, :previous_label => '«', :next_label => '»', :inner_window => 1, :outer_window => 0, :renderer => WillPaginate::ActionView::BootstrapLinkRenderer
+  = 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,
+        author:              alice.person,
-        :participant_ids => [alice.contacts.first.person.id, alice.person.id],
+        participant_ids:     [alice.contacts.first.person.id, alice.person.id],
-        :subject => 'not spam',
+        subject:             "not spam",
-        :messages_attributes => [ {:author => alice.person, :text => 'cool stuff'} ]
+        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
+    it "succeeds with json" do
-      get :index, :format => :json
+      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($("#conversations_badge .badge_count").text().trim()).toEqual("1");
-        expect($(".conversation-wrapper > .conversation.selected .unread_message_count").text().trim()).toEqual("2");
+        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)