From 64faee09356b15d3a090b508ded4b2eddb8d6759 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Thu, 17 Apr 2014 16:42:40 -0400 Subject: [PATCH] Break down new topic counts by category if a digest contains many. --- app/helpers/user_notifications_helper.rb | 11 ++++++++-- app/mailers/user_notifications.rb | 22 ++++++++++++++------ app/models/topic.rb | 21 +++++++++++++++---- app/views/user_notifications/digest.html.erb | 11 +++++++++- config/locales/server.en.yml | 1 + spec/mailers/user_notifications_spec.rb | 1 + spec/models/topic_spec.rb | 6 +++--- 7 files changed, 57 insertions(+), 16 deletions(-) diff --git a/app/helpers/user_notifications_helper.rb b/app/helpers/user_notifications_helper.rb index e36dc33c6..be41b2776 100644 --- a/app/helpers/user_notifications_helper.rb +++ b/app/helpers/user_notifications_helper.rb @@ -70,8 +70,15 @@ module UserNotificationsHelper end - def email_category(category) - return "" if category.blank? || category.uncategorized? + def email_category(category, opts=nil) + opts = opts || {} + + # If there is no category, bail + return "" if category.blank? + + # By default hide uncategorized + return "" if category.uncategorized? && !opts[:show_uncategorized] + result = "" if category.parent_category.present? result << " " diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb index 6c56837a3..0f992b8ed 100644 --- a/app/mailers/user_notifications.rb +++ b/app/mailers/user_notifications.rb @@ -30,6 +30,7 @@ class UserNotifications < ActionMailer::Base email_token: opts[:email_token]) end + def digest(user, opts={}) @user = user @base_url = Discourse.base_url @@ -41,19 +42,28 @@ class UserNotifications < ActionMailer::Base @last_seen_at = I18n.l(@user.last_seen_at || @user.created_at, format: :short) # A list of topics to show the user - @featured_topics = Topic.for_digest(user, min_date).to_a + @featured_topics = Topic.for_digest(user, min_date, limit: 20, top_order: true).to_a # Don't send email unless there is content in it if @featured_topics.present? - @new_topics_since_seen = Topic.listable_topics - .where("created_at > ?", min_date).count - @featured_topics.length + featured_topic_ids = @featured_topics.map(&:id) + + @new_topics_since_seen = Topic.new_since_last_seen(user, min_date, featured_topic_ids).count + if @new_topics_since_seen > 1000 + category_counts = Topic.new_since_last_seen(user, min_date, featured_topic_ids).group(:category_id).count + + @new_by_category = [] + if category_counts.present? + Category.where(id: category_counts.keys).each do |c| + @new_by_category << [c, category_counts[c.id]] + end + @new_by_category.sort_by! {|c| -c[1]} + end + end - @new_topics_since_seen = 0 if @new_topics_since_seen < 0 @featured_topics, @new_topics = @featured_topics[0..4], @featured_topics[5..-1] - @markdown_linker = MarkdownLinker.new(Discourse.base_url) - build_email user.email, from_alias: I18n.t('user_notifications.digest.from', site_name: SiteSetting.title), subject: I18n.t('user_notifications.digest.subject_template', diff --git a/app/models/topic.rb b/app/models/topic.rb index 8c97662eb..b7643d28b 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -260,21 +260,28 @@ class Topic < ActiveRecord::Base end # Returns hot topics since a date for display in email digest. - def self.for_digest(user, since) + def self.for_digest(user, since, opts=nil) + opts = opts || {} score = "#{ListController.best_period_for(since)}_score" topics = Topic .visible .secured(Guardian.new(user)) - .joins("LEFT OUTER JOIN top_topics ON top_topics.topic_id = topics.id") .joins("LEFT OUTER JOIN topic_users ON topic_users.topic_id = topics.id AND topic_users.user_id = #{user.id.to_i}") .where(closed: false, archived: false) .where("COALESCE(topic_users.notification_level, 1) <> ?", TopicUser.notification_levels[:muted]) .created_since(since) .listable_topics .includes(:category) - .order(TopicQuerySQL.order_top_for(score)) - .limit(20) + + if !!opts[:top_order] + topics = topics.joins("LEFT OUTER JOIN top_topics ON top_topics.topic_id = topics.id") + .order(TopicQuerySQL.order_top_for(score)) + end + + if opts[:limit] + topics = topics.limit(opts[:limit]) + end # Remove category topics category_topic_ids = Category.pluck(:topic_id).compact! @@ -291,6 +298,12 @@ class Topic < ActiveRecord::Base topics end + # Using the digest query, figure out what's new for a user since last seen + def self.new_since_last_seen(user, since, featured_topic_ids) + topics = Topic.for_digest(user, since) + topics.where("topics.id NOT IN (?)", featured_topic_ids) + end + def update_meta_data(data) self.meta_data = (self.meta_data || {}).merge(data.stringify_keys) save diff --git a/app/views/user_notifications/digest.html.erb b/app/views/user_notifications/digest.html.erb index bffbb6b87..f82104da0 100644 --- a/app/views/user_notifications/digest.html.erb +++ b/app/views/user_notifications/digest.html.erb @@ -45,7 +45,16 @@ <%- end -%> <%- if @new_topics_since_seen > 0 %> -

<%= t('user_notifications.digest.more_topics', last_seen_at: @last_seen_at, new_topics_since_seen: @new_topics_since_seen) %>

+ <%- if @new_by_category.present? %> +

<%= t('user_notifications.digest.more_topics_category', last_seen_at: @last_seen_at, new_topics_since_seen: @new_topics_since_seen) %>

+
+ <%- @new_by_category.each do |c| %> + <%= email_category(c[0], show_uncategorized: true) %> x <%= c[1] %> + <%- end %> +
+ <%- else %> +

<%= t('user_notifications.digest.more_topics', last_seen_at: @last_seen_at, new_topics_since_seen: @new_topics_since_seen) %>

+ <%- end %> <%- end -%> diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index c26e83e2b..7841349d5 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1361,6 +1361,7 @@ en: from: "%{site_name} digest" read_more: "Read More" more_topics: "Also, since we last saw you on %{last_seen_at} there have been %{new_topics_since_seen} new topics posted." + more_topics_category: "Also, since we last saw you on %{last_seen_at} there have been %{new_topics_since_seen} new topics posted in the following categories:" posts: one: "1 post" diff --git a/spec/mailers/user_notifications_spec.rb b/spec/mailers/user_notifications_spec.rb index 886a869cd..bcb5b29f6 100644 --- a/spec/mailers/user_notifications_spec.rb +++ b/spec/mailers/user_notifications_spec.rb @@ -52,6 +52,7 @@ describe UserNotifications do context "with new topics" do before do Topic.expects(:for_digest).returns([Fabricate(:topic, user: Fabricate(:coding_horror))]) + Topic.expects(:new_since_last_seen).returns(Topic.none) end its(:to) { should == [user.email] } diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index d2426eac8..d9addc1a5 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -1194,17 +1194,17 @@ describe Topic do let(:user) { Fabricate.build(:user) } it "returns none when there are no topics" do - Topic.for_digest(user, 1.year.ago).should be_blank + Topic.for_digest(user, 1.year.ago, top_order: true).should be_blank end it "doesn't return category topics" do Fabricate(:category) - Topic.for_digest(user, 1.year.ago).should be_blank + Topic.for_digest(user, 1.year.ago, top_order: true).should be_blank end it "returns regular topics" do topic = Fabricate(:topic) - Topic.for_digest(user, 1.year.ago).should == [topic] + Topic.for_digest(user, 1.year.ago, top_order: true).should == [topic] end end