From 560fb15d8a7f7d818028c41d9d954de3abc70b68 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Tue, 28 May 2013 14:54:00 -0400 Subject: [PATCH] Include pinned topics in category list. - removes an (n+1) query for user data - supports the preload store for the data to avoid a second request - fix a bug where uncategorizes was reporting (0, 0, 0) for topics by week, month, year --- .../discourse/models/category_list.js | 15 +- app/controllers/categories_controller.rb | 2 + app/models/category.rb | 2 + app/models/category_featured_topic.rb | 24 +-- app/models/category_list.rb | 163 +++++++++++------- app/models/topic_list.rb | 5 + .../category_detailed_serializer.rb | 6 +- app/views/categories/index.html.erb | 8 +- ...47_add_rank_to_category_featured_topics.rb | 6 + 9 files changed, 146 insertions(+), 85 deletions(-) create mode 100644 db/migrate/20130528174147_add_rank_to_category_featured_topics.rb diff --git a/app/assets/javascripts/discourse/models/category_list.js b/app/assets/javascripts/discourse/models/category_list.js index 668a85cb7..7329c5592 100644 --- a/app/assets/javascripts/discourse/models/category_list.js +++ b/app/assets/javascripts/discourse/models/category_list.js @@ -31,7 +31,10 @@ Discourse.CategoryList.reopenClass({ var uncategorized = Discourse.Category.uncategorizedInstance(); uncategorized.setProperties({ topics: c.topics, - featured_users: c.featured_users + featured_users: c.featured_users, + topics_week: c.topics_week, + topics_month: c.topics_month, + topics_year: c.topics_year }); categories.pushObject(uncategorized); } else { @@ -43,8 +46,16 @@ Discourse.CategoryList.reopenClass({ list: function(filter) { var route = this; + var finder = null; + if (filter === 'categories') { + finder = PreloadStore.getAndRemove("categories_list", function() { + return Discourse.ajax("/categories.json") + }); + } else { + finder = Discourse.ajax("/" + filter + ".json") + } - return Discourse.ajax("/" + filter + ".json").then(function(result) { + return finder.then(function(result) { var categoryList = Discourse.TopicList.create(); categoryList.set('can_create_category', result.category_list.can_create_category); categoryList.set('can_create_topic', result.category_list.can_create_topic); diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb index db8e07bcf..227cf647d 100644 --- a/app/controllers/categories_controller.rb +++ b/app/controllers/categories_controller.rb @@ -9,6 +9,8 @@ class CategoriesController < ApplicationController def index @list = CategoryList.new(guardian) discourse_expires_in 1.minute + + store_preloaded("categories_list", MultiJson.dump(CategoryListSerializer.new(@list, scope: guardian))) respond_to do |format| format.html { render } format.json { render_serialized(@list, CategoryListSerializer) } diff --git a/app/models/category.rb b/app/models/category.rb index 104c6a3b4..bdeacedb5 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -62,6 +62,8 @@ class Category < ActiveRecord::Base topics_week = (#{topics_week})") end + attr_accessor :displayable_topics + # Internal: Generate the text of post prompting to enter category # description. def self.post_template diff --git a/app/models/category_featured_topic.rb b/app/models/category_featured_topic.rb index 944601421..2bd614391 100644 --- a/app/models/category_featured_topic.rb +++ b/app/models/category_featured_topic.rb @@ -16,22 +16,14 @@ class CategoryFeaturedTopic < ActiveRecord::Base return if c.blank? CategoryFeaturedTopic.transaction do - exec_sql "DELETE FROM category_featured_topics WHERE category_id = :category_id", category_id: c.id - exec_sql "INSERT INTO category_featured_topics (category_id, topic_id, created_at, updated_at) - SELECT :category_id, - ft.id, - CURRENT_TIMESTAMP, - CURRENT_TIMESTAMP - FROM topics AS ft - WHERE ft.category_id = :category_id - AND ft.visible - AND ft.deleted_at IS NULL - AND ft.archetype <> :private_message - ORDER BY ft.bumped_at DESC - LIMIT :featured_limit", - category_id: c.id, - private_message: Archetype.private_message, - featured_limit: SiteSetting.category_featured_topics + CategoryFeaturedTopic.delete_all(category_id: c.id) + query = TopicQuery.new(nil, per_page: SiteSetting.category_featured_topics) + results = query.list_category(c) + if results.present? + results.topic_ids.each_with_index do |topic_id, idx| + c.category_featured_topics.create(topic_id: topic_id, rank: idx) + end + end end end diff --git a/app/models/category_list.rb b/app/models/category_list.rb index e0c21b80c..a7c10b524 100644 --- a/app/models/category_list.rb +++ b/app/models/category_list.rb @@ -3,81 +3,118 @@ class CategoryList attr_accessor :categories, :topic_users, :uncategorized - def initialize(guardian) - guardian ||= Guardian.new + def initialize(guardian=nil) + @guardian = guardian || Guardian.new - @categories = Category - .includes(featured_topics: [:category]) - .includes(:featured_users) - .where('topics.visible' => true) - .secured(guardian) - .order('categories.topics_week desc, categories.topics_month desc, categories.topics_year desc') + find_relevant_topics + find_categories + add_uncategorized + find_user_data + end + private - @categories = @categories.to_a + # Retrieve a list of all the topics we'll need + def find_relevant_topics + @topics_by_category_id = {} + category_featured_topics = CategoryFeaturedTopic.select([:category_id, :topic_id]).order(:rank) + @topics_by_id = {} - # Support for uncategorized topics - uncategorized_topics = Topic - .listable_topics - .where(category_id: nil) - .topic_list_order - .limit(SiteSetting.category_featured_topics) - if uncategorized_topics.present? - - totals = Topic.exec_sql("SELECT SUM(CASE WHEN created_at >= (CURRENT_TIMESTAMP - INTERVAL '1 WEEK') THEN 1 ELSE 0 END) as topics_week, - SUM(CASE WHEN created_at >= (CURRENT_TIMESTAMP - INTERVAL '1 MONTH') THEN 1 ELSE 0 END) as topics_month, - SUM(CASE WHEN created_at >= (CURRENT_TIMESTAMP - INTERVAL '1 YEAR') THEN 1 ELSE 0 END) as topics_year, - COUNT(*) AS topic_count - FROM topics - WHERE topics.visible - AND topics.deleted_at IS NULL - AND topics.category_id IS NULL - AND topics.archetype <> '#{Archetype.private_message}'").first - - - uncategorized = Category.new({name: SiteSetting.uncategorized_name, - slug: Slug.for(SiteSetting.uncategorized_name), - color: SiteSetting.uncategorized_color, - text_color: SiteSetting.uncategorized_text_color, - featured_topics: uncategorized_topics}.merge(totals)) - - # Find the appropriate place to insert it: - insert_at = nil - @categories.each_with_index do |c, idx| - if totals['topics_week'].to_i > (c.topics_week || 0) - insert_at = idx - break - end + @all_topics = Topic.where(id: category_featured_topics.map(&:topic_id)) + @all_topics.each do |t| + @topics_by_id[t.id] = t end - @categories.insert(insert_at || @categories.size, uncategorized) + category_featured_topics.each do |cft| + @topics_by_category_id[cft.category_id] ||= [] + @topics_by_category_id[cft.category_id] << cft.topic_id + end end - unless guardian.can_create?(Category) - # Remove categories with no featured topics unless we have the ability to edit one - @categories.delete_if { |c| c.featured_topics.blank? } - else - # Show all categories to people who have the ability to edit and delete categories - if @categories.size > 0 - @categories.insert(@categories.size, *Category.where('id not in (?)', @categories.map(&:id).compact).to_a) - else - @categories = Category.all.to_a + # Find a list of all categories to associate the topics with + def find_categories + @categories = Category + .includes(:featured_users) + .secured(@guardian) + .order('COALESCE(categories.topics_week, 0) DESC') + .order('COALESCE(categories.topics_month, 0) DESC') + .order('COALESCE(categories.topics_year, 0) DESC') + + @categories = @categories.to_a + @categories.each do |c| + topics_in_cat = @topics_by_category_id[c.id] + if topics_in_cat.present? + c.displayable_topics = [] + topics_in_cat.each do |topic_id| + topic = @topics_by_id[topic_id] + topic.category = c + c.displayable_topics << topic + end + end + end + end + + # Add the uncategorized "magic" category + def add_uncategorized + # Support for uncategorized topics + uncategorized_topics = Topic + .listable_topics + .where(category_id: nil) + .topic_list_order + .limit(SiteSetting.category_featured_topics) + if uncategorized_topics.present? + + totals = Topic.exec_sql("SELECT SUM(CASE WHEN created_at >= (CURRENT_TIMESTAMP - INTERVAL '1 WEEK') THEN 1 ELSE 0 END) as topics_week, + SUM(CASE WHEN created_at >= (CURRENT_TIMESTAMP - INTERVAL '1 MONTH') THEN 1 ELSE 0 END) as topics_month, + SUM(CASE WHEN created_at >= (CURRENT_TIMESTAMP - INTERVAL '1 YEAR') THEN 1 ELSE 0 END) as topics_year, + COUNT(*) AS topic_count + FROM topics + WHERE topics.visible + AND topics.deleted_at IS NULL + AND topics.category_id IS NULL + AND topics.archetype <> '#{Archetype.private_message}'").first + + + uncategorized = Category.new({name: SiteSetting.uncategorized_name, + slug: Slug.for(SiteSetting.uncategorized_name), + color: SiteSetting.uncategorized_color, + text_color: SiteSetting.uncategorized_text_color, + featured_topics: uncategorized_topics}.merge(totals)) + + # Find the appropriate place to insert it: + insert_at = nil + @categories.each_with_index do |c, idx| + if uncategorized.topics_week > (c.topics_week || 0) + insert_at = idx + break + end + end + + @categories.insert(insert_at || @categories.size, uncategorized) + end + + if @all_topics.present? + uncategorized.displayable_topics = uncategorized_topics + @all_topics << uncategorized_topics + @all_topics.flatten! + end + end + + # Remove any empty topics unless we can create them (so we can see the controls) + def prune_empty + unless @guardian.can_create?(Category) + # Remove categories with no featured topics unless we have the ability to edit one + @categories.delete_if { |c| c.displayable_topics.blank? } end end # Get forum topic user records if appropriate - if guardian.current_user - topics = [] - @categories.each { |c| topics << c.featured_topics } - topics << @uncategorized + def find_user_data + if @guardian.current_user && @all_topics.present? + topic_lookup = TopicUser.lookup_for(@guardian.current_user, @all_topics) - topics.flatten! if topics.present? - topics.compact! if topics.present? - - topic_lookup = TopicUser.lookup_for(guardian.current_user, topics) - - # Attach some data for serialization to each topic - topics.each { |ft| ft.user_data = topic_lookup[ft.id] } + # Attach some data for serialization to each topic + @all_topics.each { |ft| ft.user_data = topic_lookup[ft.id] } + end end - end end diff --git a/app/models/topic_list.rb b/app/models/topic_list.rb index 916b5b6df..f66f7ab8e 100644 --- a/app/models/topic_list.rb +++ b/app/models/topic_list.rb @@ -41,6 +41,11 @@ class TopicList return @topics end + def topic_ids + return [] if @topics_input.blank? + @topics_input.map {|t| t.id} + end + def filter_summary @filter_summary ||= get_summary end diff --git a/app/serializers/category_detailed_serializer.rb b/app/serializers/category_detailed_serializer.rb index 38c811f13..a7a51c001 100644 --- a/app/serializers/category_detailed_serializer.rb +++ b/app/serializers/category_detailed_serializer.rb @@ -13,7 +13,7 @@ class CategoryDetailedSerializer < ApplicationSerializer :is_uncategorized has_many :featured_users, serializer: BasicUserSerializer - has_many :featured_topics, serializer: CategoryTopicSerializer, embed: :objects, key: :topics + has_many :displayable_topics, serializer: CategoryTopicSerializer, embed: :objects, key: :topics def topics_week object.topics_week || 0 @@ -35,4 +35,8 @@ class CategoryDetailedSerializer < ApplicationSerializer is_uncategorized end + def include_displayable_topics? + return displayable_topics.present? + end + end diff --git a/app/views/categories/index.html.erb b/app/views/categories/index.html.erb index e2b388dcb..1818e4d06 100644 --- a/app/views/categories/index.html.erb +++ b/app/views/categories/index.html.erb @@ -2,9 +2,11 @@

<%= c.name %>

- <% c.topics.limit(6).each do |t| %> - <%= t.title %> '>(<%= t.posts_count %>)
- <% end %> + <%- if c.displayable_topics.present? %> + <% c.displayable_topics.each do |t| %> + <%= t.title %> '>(<%= t.posts_count %>)
+ <% end %> + <%- end %>
<% end %> diff --git a/db/migrate/20130528174147_add_rank_to_category_featured_topics.rb b/db/migrate/20130528174147_add_rank_to_category_featured_topics.rb new file mode 100644 index 000000000..ea9c1c4e9 --- /dev/null +++ b/db/migrate/20130528174147_add_rank_to_category_featured_topics.rb @@ -0,0 +1,6 @@ +class AddRankToCategoryFeaturedTopics < ActiveRecord::Migration + def change + add_column :category_featured_topics, :rank, :integer, default: 0, null: false + add_index :category_featured_topics, [:category_id, :rank] + end +end