From 010afa101951622dbef505b5040dbd2c366b4a3a Mon Sep 17 00:00:00 2001 From: Benjamin Neff Date: Fri, 5 Jun 2015 02:58:15 +0200 Subject: [PATCH] refactor: iterate over visiblities closes #6060 --- Changelog.md | 1 + app/controllers/conversations_controller.rb | 14 +------ app/models/conversation.rb | 8 +++- app/views/conversations/_conversation.haml | 15 ++++---- .../conversations/_conversation.mobile.haml | 12 +++--- app/views/conversations/index.haml | 11 +++--- app/views/conversations/index.mobile.haml | 7 ++-- .../conversations_controller_spec.rb | 38 ++++++++++++------- .../app/views/conversations_view_spec.js | 13 ++++--- spec/models/conversation_spec.rb | 9 +++++ 10 files changed, 73 insertions(+), 55 deletions(-) diff --git a/Changelog.md b/Changelog.md index 8c0856f2b..74b7a6a6c 100644 --- a/Changelog.md +++ b/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) diff --git a/app/controllers/conversations_controller.rb b/app/controllers/conversations_controller.rb index 278065734..b4fb495d4 100644 --- a/app/controllers/conversations_controller.rb +++ b/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 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/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 7c30d0e1f..90378c45f 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) .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 9f28747ad..e6f09a4c9 100644 --- a/app/views/conversations/index.haml +++ b/app/views/conversations/index.haml @@ -10,18 +10,19 @@ #left_pane #left_pane_header %h3 - .pull-right{ :class => ("hidden" unless @conversation)} - = link_to t('.new_conversation'), conversations_path, :class => 'btn btn-default' + .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, 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 diff --git a/app/views/conversations/index.mobile.haml b/app/views/conversations/index.mobile.haml index 57341e0d5..885bfd38c 100644 --- a/app/views/conversations/index.mobile.haml +++ b/app/views/conversations/index.mobile.haml @@ -16,8 +16,8 @@ #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, 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 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)