diff --git a/app/models/topic.rb b/app/models/topic.rb index 17616895744..6b606d04026 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -508,60 +508,8 @@ class Topic < ActiveRecord::Base save end - # Create the summary of the interesting posters in a topic. Cheats to avoid - # many queries. - def posters_summary(topic_user = nil, current_user = nil, opts={}) - return @posters_summary if @posters_summary.present? - descriptions = {} - - # Use an avatar lookup object if we have it, otherwise create one just for this forum topic - al = opts[:avatar_lookup] - if al.blank? - al = AvatarLookup.new([user_id, last_post_user_id, featured_user1_id, featured_user2_id, featured_user3_id]) - end - - # Helps us add a description to a poster - add_description = lambda do |u, desc| - if u.present? - descriptions[u.id] ||= [] - descriptions[u.id] << I18n.t(desc) - end - end - - add_description.call(al[user_id], :original_poster) - add_description.call(al[featured_user1_id], :most_posts) - add_description.call(al[featured_user2_id], :frequent_poster) - add_description.call(al[featured_user3_id], :frequent_poster) - add_description.call(al[featured_user4_id], :frequent_poster) - add_description.call(al[last_post_user_id], :most_recent_poster) - - - @posters_summary = [al[user_id], - al[last_post_user_id], - al[featured_user1_id], - al[featured_user2_id], - al[featured_user3_id], - al[featured_user4_id] - ].compact.uniq[0..4] - - unless @posters_summary[0] == al[last_post_user_id] - # shuffle last_poster to back - @posters_summary.reject!{|u| u == al[last_post_user_id]} - @posters_summary << al[last_post_user_id] - end - @posters_summary.map! do |p| - if p - result = TopicPoster.new - result.user = p - result.description = descriptions[p.id].join(', ') - result.extras = "latest" if al[last_post_user_id] == p - result - else - nil - end - end.compact! - - @posters_summary + def posters_summary(options = {}) + @posters_summary ||= TopicPostersSummary.new(self, options).summary end # Enable/disable the star on the topic diff --git a/app/models/topic_list.rb b/app/models/topic_list.rb index 1f87bfcb6f3..916b5b6dfe2 100644 --- a/app/models/topic_list.rb +++ b/app/models/topic_list.rb @@ -34,7 +34,7 @@ class TopicList @topics.each do |ft| ft.user_data = @topic_lookup[ft.id] if @topic_lookup.present? - ft.posters = ft.posters_summary(ft.user_data, @current_user, avatar_lookup: avatar_lookup) + ft.posters = ft.posters_summary(avatar_lookup: avatar_lookup) ft.topic_list = self end diff --git a/app/models/topic_posters_summary.rb b/app/models/topic_posters_summary.rb new file mode 100644 index 00000000000..b3c0e42ae10 --- /dev/null +++ b/app/models/topic_posters_summary.rb @@ -0,0 +1,79 @@ +class TopicPostersSummary + attr_reader :topic, :options + + def initialize(topic, options = {}) + @topic = topic + @options = options + end + + def summary + sorted_top_posters.map { |user| new_topic_poster_for user } + end + + private + + def new_topic_poster_for(user) + TopicPoster.new.tap do |topic_poster| + topic_poster.user = user + topic_poster.description = descriptions_for(user) + topic_poster.extras = 'latest' if is_latest_poster?(user) + end + end + + def descriptions_by_id + @descriptions_by_id ||= begin + user_ids_with_descriptions.inject({}) do |descriptions, (id, description)| + descriptions[id] ||= [] + descriptions[id] << description + descriptions + end + end + end + + def descriptions_for(user) + descriptions_by_id[user.id].join ', ' + end + + def shuffle_last_poster_to_back_in(summary) + unless last_poster_is_topic_creator? + summary.reject!{ |u| u.id == topic.last_post_user_id } + summary << avatar_lookup[topic.last_post_user_id] + end + summary + end + + def user_ids_with_descriptions + user_ids.zip([ + :original_poster, + :most_recent_poster, + :most_posts, + :frequent_poster, + :frequent_poster, + :frequent_poster + ].map { |description| I18n.t(description) }) + end + + def last_poster_is_topic_creator? + topic.user_id == topic.last_post_user_id + end + + def is_latest_poster?(user) + topic.last_post_user_id == user.id + end + + def sorted_top_posters + shuffle_last_poster_to_back_in top_posters + end + + def top_posters + user_ids.map { |id| avatar_lookup[id] }.compact.uniq.take(5) + end + + def user_ids + [ topic.user_id, topic.last_post_user_id, *topic.featured_user_ids ] + end + + def avatar_lookup + @avatar_lookup ||= options[:avatar_lookup] || AvatarLookup.new(user_ids) + end +end diff --git a/spec/models/topic_posters_summary_spec.rb b/spec/models/topic_posters_summary_spec.rb new file mode 100644 index 00000000000..06d02f9db13 --- /dev/null +++ b/spec/models/topic_posters_summary_spec.rb @@ -0,0 +1,71 @@ +require 'spec_helper' + +describe TopicPostersSummary do + describe '#summary' do + let(:summary) { described_class.new(topic).summary } + + let(:topic) do + Fabricate(:topic, + user: topic_creator, + last_poster: last_poster, + featured_user1: featured_user1, + featured_user2: featured_user2, + featured_user3: featured_user3, + featured_user4: featured_user4 + ) + end + + let(:topic_creator) { Fabricate(:user) } + let(:last_poster) { nil } + let(:featured_user1) { nil } + let(:featured_user2) { nil } + let(:featured_user3) { nil } + let(:featured_user4) { nil } + + it 'contains only the topic creator when there are no other posters' do + expect(summary.count).to eq 1 + + summary.first.tap do |topic_poster| + expect(topic_poster.user).to eq topic_creator + expect(topic_poster.extras).to eq 'latest' + expect(topic_poster.description).to eq( + "#{I18n.t(:original_poster)}, #{I18n.t(:most_recent_poster)}" + ) + end + end + + context 'when the lastest poster is also the topic creator' do + let(:last_poster) { topic_creator } + let(:featured_user1) { Fabricate(:user) } + + before do + topic.last_poster = topic_creator + topic.featured_user1 = featured_user1 + topic.save! + end + + it 'keeps the topic creator at the front of the summary' do + expect(summary.map(&:user)).to eq([ + topic_creator, + featured_user1 + ]) + end + end + + context 'when the topic has many posters' do + let(:last_poster) { Fabricate(:user) } + let(:featured_user1) { Fabricate(:user) } + let(:featured_user2) { Fabricate(:user) } + let(:featured_user3) { Fabricate(:user) } + let(:featured_user4) { Fabricate(:user) } + + it 'contains only five posters with latest poster at the end' do + expect(summary.map(&:user)).to eq([ + topic_creator, + featured_user1, featured_user2, featured_user3, + last_poster + ]) + end + end + end +end