diff --git a/Changelog.md b/Changelog.md index dc473f4af..39d1009fc 100644 --- a/Changelog.md +++ b/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) diff --git a/app/controllers/conversations_controller.rb b/app/controllers/conversations_controller.rb index 7edf04aee..b4fb495d4 100644 --- a/app/controllers/conversations_controller.rb +++ b/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 diff --git a/app/models/conversation.rb b/app/models/conversation.rb index dbf1fb774..a4053a212 100644 --- a/app/models/conversation.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index e7685cd6e..d64708a0e 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/views/conversations/_conversation.haml b/app/views/conversations/_conversation.haml index 0fa3d5cff..17899a543 100644 --- a/app/views/conversations/_conversation.haml +++ b/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 diff --git a/app/views/conversations/_conversation.mobile.haml b/app/views/conversations/_conversation.mobile.haml index 59ed20290..cce4df2ef 100644 --- a/app/views/conversations/_conversation.mobile.haml +++ b/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 diff --git a/app/views/conversations/index.haml b/app/views/conversations/index.haml index 27b65a213..660ced939 100644 --- a/app/views/conversations/index.haml +++ b/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 diff --git a/app/views/conversations/index.mobile.haml b/app/views/conversations/index.mobile.haml index 510008d5a..05a941904 100644 --- a/app/views/conversations/index.mobile.haml +++ b/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 diff --git a/spec/controllers/conversations_controller_spec.rb b/spec/controllers/conversations_controller_spec.rb index fe1cb3263..868649c62 100644 --- a/spec/controllers/conversations_controller_spec.rb +++ b/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 diff --git a/spec/javascripts/app/views/conversations_view_spec.js b/spec/javascripts/app/views/conversations_view_spec.js index 898f88152..44abb5022 100644 --- a/spec/javascripts/app/views/conversations_view_spec.js +++ b/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 = "
3
"; $("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 = "
2
"; + var badge = "
1
"; $("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"); diff --git a/spec/models/conversation_spec.rb b/spec/models/conversation_spec.rb index 4e555bf2e..78425bc46 100644 --- a/spec/models/conversation_spec.rb +++ b/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)