From 94d8f6d7342e862a38b9d01d7d9bac556dd038b2 Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Mon, 14 Aug 2017 12:47:33 -0400 Subject: [PATCH] FIX: digest emails should not include posts that are still in the edit grace period --- app/mailers/user_notifications.rb | 1 + app/models/topic.rb | 1 + spec/jobs/user_email_spec.rb | 5 +- spec/mailers/user_notifications_spec.rb | 30 ++-- spec/models/topic_spec.rb | 228 +++++++++++++----------- 5 files changed, 148 insertions(+), 117 deletions(-) diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb index 049e6658275..c4ba2c5532c 100644 --- a/app/mailers/user_notifications.rb +++ b/app/mailers/user_notifications.rb @@ -99,6 +99,7 @@ class UserNotifications < ActionMailer::Base .where('posts.post_type = ?', Post.types[:regular]) .where('posts.deleted_at IS NULL AND posts.hidden = false AND posts.user_deleted = false') .where("posts.post_number > ? AND posts.score > ?", 1, ScoreCalculator.default_score_weights[:like_score] * 5.0) + .where('posts.created_at < ?', (SiteSetting.editing_grace_period || 0).seconds.ago) .limit(SiteSetting.digest_posts) else [] diff --git a/app/models/topic.rb b/app/models/topic.rb index b076191a8d3..7eb6e1c554e 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -337,6 +337,7 @@ class Topic < ActiveRecord::Base .where(closed: false, archived: false) .where("COALESCE(topic_users.notification_level, 1) <> ?", TopicUser.notification_levels[:muted]) .created_since(since) + .where('topics.created_at < ?', (SiteSetting.editing_grace_period || 0).seconds.ago) .listable_topics .includes(:category) diff --git a/spec/jobs/user_email_spec.rb b/spec/jobs/user_email_spec.rb index 6f2fcf5795e..b72e167582d 100644 --- a/spec/jobs/user_email_spec.rb +++ b/spec/jobs/user_email_spec.rb @@ -80,7 +80,10 @@ describe Jobs::UserEmail do context "email_log" do - before { Fabricate(:post) } + before do + SiteSetting.editing_grace_period = 0 + Fabricate(:post) + end it "creates an email log when the mail is sent (via Email::Sender)" do last_emailed_at = user.last_emailed_at diff --git a/spec/mailers/user_notifications_spec.rb b/spec/mailers/user_notifications_spec.rb index 34265ec960b..37da9a05977 100644 --- a/spec/mailers/user_notifications_spec.rb +++ b/spec/mailers/user_notifications_spec.rb @@ -106,7 +106,7 @@ describe UserNotifications do context "with new topics" do before do - Fabricate(:topic, user: Fabricate(:coding_horror)) + Fabricate(:topic, user: Fabricate(:coding_horror), created_at: 1.hour.ago) end it "works" do @@ -127,8 +127,8 @@ describe UserNotifications do end it "excludes deleted topics and their posts" do - deleted = Fabricate(:topic, user: Fabricate(:user), title: "Delete this topic plz") - post = Fabricate(:post, topic: deleted, score: 100.0, post_number: 2, raw: "Your wish is my command") + deleted = Fabricate(:topic, user: Fabricate(:user), title: "Delete this topic plz", created_at: 1.hour.ago) + post = Fabricate(:post, topic: deleted, score: 100.0, post_number: 2, raw: "Your wish is my command", created_at: 1.hour.ago) deleted.trash! html = subject.html_part.body.to_s expect(html).to_not include deleted.title @@ -136,10 +136,10 @@ describe UserNotifications do end it "excludes whispers and other post types that don't belong" do - t = Fabricate(:topic, user: Fabricate(:user), title: "Who likes the same stuff I like?") - whisper = Fabricate(:post, topic: t, score: 100.0, post_number: 2, raw: "You like weird stuff", post_type: Post.types[:whisper]) - mod_action = Fabricate(:post, topic: t, score: 100.0, post_number: 3, raw: "This topic unlisted", post_type: Post.types[:moderator_action]) - small_action = Fabricate(:post, topic: t, score: 100.0, post_number: 4, raw: "A small action", post_type: Post.types[:small_action]) + t = Fabricate(:topic, user: Fabricate(:user), title: "Who likes the same stuff I like?", created_at: 1.hour.ago) + whisper = Fabricate(:post, topic: t, score: 100.0, post_number: 2, raw: "You like weird stuff", post_type: Post.types[:whisper], created_at: 1.hour.ago) + mod_action = Fabricate(:post, topic: t, score: 100.0, post_number: 3, raw: "This topic unlisted", post_type: Post.types[:moderator_action], created_at: 1.hour.ago) + small_action = Fabricate(:post, topic: t, score: 100.0, post_number: 4, raw: "A small action", post_type: Post.types[:small_action], created_at: 1.hour.ago) html = subject.html_part.body.to_s expect(html).to_not include whisper.raw expect(html).to_not include mod_action.raw @@ -147,16 +147,24 @@ describe UserNotifications do end it "excludes deleted and hidden posts" do - t = Fabricate(:topic, user: Fabricate(:user), title: "Post objectionable stuff here") - deleted = Fabricate(:post, topic: t, score: 100.0, post_number: 2, raw: "This post is uncalled for", deleted_at: 5.minutes.ago) - hidden = Fabricate(:post, topic: t, score: 100.0, post_number: 3, raw: "Try to find this post", hidden: true, hidden_at: 5.minutes.ago, hidden_reason_id: Post.hidden_reasons[:flagged_by_tl3_user]) - user_deleted = Fabricate(:post, topic: t, score: 100.0, post_number: 4, raw: "I regret this post", user_deleted: true) + t = Fabricate(:topic, user: Fabricate(:user), title: "Post objectionable stuff here", created_at: 1.hour.ago) + deleted = Fabricate(:post, topic: t, score: 100.0, post_number: 2, raw: "This post is uncalled for", deleted_at: 5.minutes.ago, created_at: 1.hour.ago) + hidden = Fabricate(:post, topic: t, score: 100.0, post_number: 3, raw: "Try to find this post", hidden: true, hidden_at: 5.minutes.ago, hidden_reason_id: Post.hidden_reasons[:flagged_by_tl3_user], created_at: 1.hour.ago) + user_deleted = Fabricate(:post, topic: t, score: 100.0, post_number: 4, raw: "I regret this post", user_deleted: true, created_at: 1.hour.ago) html = subject.html_part.body.to_s expect(html).to_not include deleted.raw expect(html).to_not include hidden.raw expect(html).to_not include user_deleted.raw end + it "excludes posts that are newer than editing grace period" do + SiteSetting.editing_grace_period = 5.minutes + too_new = Fabricate(:topic, user: Fabricate(:user), title: "Oops I need to edit this", created_at: 1.minute.ago) + too_new_post = Fabricate(:post, user: too_new.user, topic: too_new, score: 100.0, post_number: 1, created_at: 1.minute.ago) + html = subject.html_part.body.to_s + expect(html).to_not include too_new.title + end + it "uses theme color" do cs = Fabricate(:color_scheme, name: 'Fancy', color_scheme_colors: [ Fabricate(:color_scheme_color, name: 'header_primary', hex: 'F0F0F0'), diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index a666a817028..a1d5334fce6 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -1327,113 +1327,131 @@ describe Topic do describe 'for_digest' do let(:user) { Fabricate.build(:user) } - it "returns none when there are no topics" do - expect(Topic.for_digest(user, 1.year.ago, top_order: true)).to be_blank + context "no edit grace period" do + before do + SiteSetting.editing_grace_period = 0 + end + + it "returns none when there are no topics" do + expect(Topic.for_digest(user, 1.year.ago, top_order: true)).to be_blank + end + + it "doesn't return category topics" do + Fabricate(:category) + expect(Topic.for_digest(user, 1.year.ago, top_order: true)).to be_blank + end + + it "returns regular topics" do + topic = Fabricate(:topic) + expect(Topic.for_digest(user, 1.year.ago, top_order: true)).to eq([topic]) + end + + it "doesn't return topics from muted categories" do + user = Fabricate(:user) + category = Fabricate(:category) + Fabricate(:topic, category: category) + + CategoryUser.set_notification_level_for_category(user, CategoryUser.notification_levels[:muted], category.id) + + expect(Topic.for_digest(user, 1.year.ago, top_order: true)).to be_blank + end + + it "doesn't return topics from suppressed categories" do + user = Fabricate(:user) + category = Fabricate(:category) + Fabricate(:topic, category: category) + + SiteSetting.digest_suppress_categories = "#{category.id}" + + expect(Topic.for_digest(user, 1.year.ago, top_order: true)).to be_blank + end + + it "doesn't return topics from TL0 users" do + new_user = Fabricate(:user, trust_level: 0) + Fabricate(:topic, user_id: new_user.id) + + expect(Topic.for_digest(user, 1.year.ago, top_order: true)).to be_blank + end + + it "returns topics from TL0 users if given include_tl0" do + new_user = Fabricate(:user, trust_level: 0) + topic = Fabricate(:topic, user_id: new_user.id) + + expect(Topic.for_digest(user, 1.year.ago, top_order: true, include_tl0: true)).to eq([topic]) + end + + it "returns topics from TL0 users if enabled in preferences" do + new_user = Fabricate(:user, trust_level: 0) + topic = Fabricate(:topic, user_id: new_user.id) + + u = Fabricate(:user) + u.user_option.include_tl0_in_digests = true + + expect(Topic.for_digest(u, 1.year.ago, top_order: true)).to eq([topic]) + end + + it "doesn't return topics with only muted tags" do + user = Fabricate(:user) + tag = Fabricate(:tag) + TagUser.change(user.id, tag.id, TagUser.notification_levels[:muted]) + Fabricate(:topic, tags: [tag]) + + expect(Topic.for_digest(user, 1.year.ago, top_order: true)).to be_blank + end + + it "returns topics with both muted and not muted tags" do + user = Fabricate(:user) + muted_tag, other_tag = Fabricate(:tag), Fabricate(:tag) + TagUser.change(user.id, muted_tag.id, TagUser.notification_levels[:muted]) + topic = Fabricate(:topic, tags: [muted_tag, other_tag]) + + expect(Topic.for_digest(user, 1.year.ago, top_order: true)).to eq([topic]) + end + + it "returns topics with no tags too" do + user = Fabricate(:user) + muted_tag = Fabricate(:tag) + TagUser.change(user.id, muted_tag.id, TagUser.notification_levels[:muted]) + topic1 = Fabricate(:topic, tags: [muted_tag]) + topic2 = Fabricate(:topic, tags: [Fabricate(:tag), Fabricate(:tag)]) + topic3 = Fabricate(:topic) + + topics = Topic.for_digest(user, 1.year.ago, top_order: true) + expect(topics.size).to eq(2) + expect(topics).to contain_exactly(topic2, topic3) + end + + it "sorts by category notification levels" do + category1, category2 = Fabricate(:category), Fabricate(:category) + 2.times { |i| Fabricate(:topic, category: category1) } + topic1 = Fabricate(:topic, category: category2) + 2.times { |i| Fabricate(:topic, category: category1) } + CategoryUser.create(user: user, category: category2, notification_level: CategoryUser.notification_levels[:watching]) + for_digest = Topic.for_digest(user, 1.year.ago, top_order: true) + expect(for_digest.first).to eq(topic1) + end + + it "sorts by topic notification levels" do + topics = [] + 3.times { |i| topics << Fabricate(:topic) } + user = Fabricate(:user) + TopicUser.create(user_id: user.id, topic_id: topics[0].id, notification_level: TopicUser.notification_levels[:tracking]) + TopicUser.create(user_id: user.id, topic_id: topics[2].id, notification_level: TopicUser.notification_levels[:watching]) + for_digest = Topic.for_digest(user, 1.year.ago, top_order: true).pluck(:id) + expect(for_digest).to eq([topics[2].id, topics[0].id, topics[1].id]) + end end - it "doesn't return category topics" do - Fabricate(:category) - expect(Topic.for_digest(user, 1.year.ago, top_order: true)).to be_blank - end + context "with editing_grace_period" do + before do + SiteSetting.editing_grace_period = 5.minutes + end - it "returns regular topics" do - topic = Fabricate(:topic) - expect(Topic.for_digest(user, 1.year.ago, top_order: true)).to eq([topic]) - end - - it "doesn't return topics from muted categories" do - user = Fabricate(:user) - category = Fabricate(:category) - Fabricate(:topic, category: category) - - CategoryUser.set_notification_level_for_category(user, CategoryUser.notification_levels[:muted], category.id) - - expect(Topic.for_digest(user, 1.year.ago, top_order: true)).to be_blank - end - - it "doesn't return topics from suppressed categories" do - user = Fabricate(:user) - category = Fabricate(:category) - Fabricate(:topic, category: category) - - SiteSetting.digest_suppress_categories = "#{category.id}" - - expect(Topic.for_digest(user, 1.year.ago, top_order: true)).to be_blank - end - - it "doesn't return topics from TL0 users" do - new_user = Fabricate(:user, trust_level: 0) - Fabricate(:topic, user_id: new_user.id) - - expect(Topic.for_digest(user, 1.year.ago, top_order: true)).to be_blank - end - - it "returns topics from TL0 users if given include_tl0" do - new_user = Fabricate(:user, trust_level: 0) - topic = Fabricate(:topic, user_id: new_user.id) - - expect(Topic.for_digest(user, 1.year.ago, top_order: true, include_tl0: true)).to eq([topic]) - end - - it "returns topics from TL0 users if enabled in preferences" do - new_user = Fabricate(:user, trust_level: 0) - topic = Fabricate(:topic, user_id: new_user.id) - - u = Fabricate(:user) - u.user_option.include_tl0_in_digests = true - - expect(Topic.for_digest(u, 1.year.ago, top_order: true)).to eq([topic]) - end - - it "doesn't return topics with only muted tags" do - user = Fabricate(:user) - tag = Fabricate(:tag) - TagUser.change(user.id, tag.id, TagUser.notification_levels[:muted]) - Fabricate(:topic, tags: [tag]) - - expect(Topic.for_digest(user, 1.year.ago, top_order: true)).to be_blank - end - - it "returns topics with both muted and not muted tags" do - user = Fabricate(:user) - muted_tag, other_tag = Fabricate(:tag), Fabricate(:tag) - TagUser.change(user.id, muted_tag.id, TagUser.notification_levels[:muted]) - topic = Fabricate(:topic, tags: [muted_tag, other_tag]) - - expect(Topic.for_digest(user, 1.year.ago, top_order: true)).to eq([topic]) - end - - it "returns topics with no tags too" do - user = Fabricate(:user) - muted_tag = Fabricate(:tag) - TagUser.change(user.id, muted_tag.id, TagUser.notification_levels[:muted]) - topic1 = Fabricate(:topic, tags: [muted_tag]) - topic2 = Fabricate(:topic, tags: [Fabricate(:tag), Fabricate(:tag)]) - topic3 = Fabricate(:topic) - - topics = Topic.for_digest(user, 1.year.ago, top_order: true) - expect(topics.size).to eq(2) - expect(topics).to contain_exactly(topic2, topic3) - end - - it "sorts by category notification levels" do - category1, category2 = Fabricate(:category), Fabricate(:category) - 2.times { |i| Fabricate(:topic, category: category1) } - topic1 = Fabricate(:topic, category: category2) - 2.times { |i| Fabricate(:topic, category: category1) } - CategoryUser.create(user: user, category: category2, notification_level: CategoryUser.notification_levels[:watching]) - for_digest = Topic.for_digest(user, 1.year.ago, top_order: true) - expect(for_digest.first).to eq(topic1) - end - - it "sorts by topic notification levels" do - topics = [] - 3.times { |i| topics << Fabricate(:topic) } - user = Fabricate(:user) - TopicUser.create(user_id: user.id, topic_id: topics[0].id, notification_level: TopicUser.notification_levels[:tracking]) - TopicUser.create(user_id: user.id, topic_id: topics[2].id, notification_level: TopicUser.notification_levels[:watching]) - for_digest = Topic.for_digest(user, 1.year.ago, top_order: true).pluck(:id) - expect(for_digest).to eq([topics[2].id, topics[0].id, topics[1].id]) + it "excludes topics that are within the grace period" do + topic1 = Fabricate(:topic, created_at: 6.minutes.ago) + topic2 = Fabricate(:topic, created_at: 4.minutes.ago) + expect(Topic.for_digest(user, 1.year.ago, top_order: true)).to eq([topic1]) + end end end @@ -1441,7 +1459,7 @@ describe Topic do describe 'secured' do it 'can remove secure groups' do category = Fabricate(:category, read_restricted: true) - Fabricate(:topic, category: category) + Fabricate(:topic, category: category, created_at: 1.day.ago) expect(Topic.secured(Guardian.new(nil)).count).to eq(0) expect(Topic.secured(Guardian.new(Fabricate(:admin))).count).to eq(2)