From e2f190512ba99ab6600e50593f2a4c3019d28041 Mon Sep 17 00:00:00 2001 From: Josh Puetz Date: Fri, 6 Mar 2020 12:29:07 -0600 Subject: [PATCH] Guarantee articles in main article feed (#6490) [deploy] * Adjust feed offset, remove time limits for main feed * Improve offset construction readability --- app/services/articles/feed.rb | 6 ++- spec/services/articles/feed_spec.rb | 76 +++++++++++++++++++++++------ 2 files changed, 64 insertions(+), 18 deletions(-) diff --git a/app/services/articles/feed.rb b/app/services/articles/feed.rb index 275015ae1..bf3e3ccbd 100644 --- a/app/services/articles/feed.rb +++ b/app/services/articles/feed.rb @@ -1,5 +1,7 @@ module Articles class Feed + RANDOM_OFFSET_VALUES = [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 2, 2, 2, 3, 3, 4, 5, 6, 7, 8, 9, 10, 11].freeze + def initialize(user: nil, number_of_articles: 35, page: 1, tag: nil) @user = user @number_of_articles = number_of_articles @@ -137,10 +139,10 @@ module Articles order("hotness_score DESC") featured_story = hot_stories.where.not(main_image: nil).first if user_signed_in - offset = [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 2, 2, 2, 3, 3, 4, 5, 6, 7, 8, 9, 10, 11].sample # random offset, weighted more towards zero + offset = RANDOM_OFFSET_VALUES.select { |i| i < hot_stories.count }.sample # random offset, weighted more towards zero hot_stories = hot_stories.offset(offset) new_stories = Article.published. - where("published_at > ? AND score > ?", rand(2..6).hours.ago, -15). + where("score > ?", -15). limited_column_select.order("published_at DESC").limit(rand(15..80)) hot_stories = hot_stories.to_a + new_stories.to_a end diff --git a/spec/services/articles/feed_spec.rb b/spec/services/articles/feed_spec.rb index 23eb44945..9e361bec3 100644 --- a/spec/services/articles/feed_spec.rb +++ b/spec/services/articles/feed_spec.rb @@ -77,12 +77,10 @@ RSpec.describe Articles::Feed, type: :service do let(:featured_story) { result.first } let(:stories) { result.second } - it "only includes stories from less than 6 hours ago" do - expect(stories).not_to include(old_story) - expect(stories).not_to include(article) - - # Ideally we'd test for hot_story in the stories list, but the random offset selection makes that random - expect(featured_story).to eq(hot_story) + it "only includes stories" do + expect(stories).to include(old_story) + expect(stories).to include(article) + expect(stories).to include(hot_story) end end @@ -132,8 +130,8 @@ RSpec.describe Articles::Feed, type: :service do context "when user logged in" do let(:stories) { feed.default_home_feed(user_signed_in: true) } - it "includes stories from between 2 and 6 hours ago" do - expect(stories).not_to include(old_story) + it "includes stories " do + expect(stories).to include(old_story) expect(stories).to include(new_story) end end @@ -143,8 +141,8 @@ RSpec.describe Articles::Feed, type: :service do let!(:new_story) { create(:article, published_at: 10.minutes.ago, score: 10) } let(:stories) { feed.default_home_feed_with_more_randomness } - it "includes stories from between 2 and 6 hours ago" do - expect(stories).not_to include(old_story) + it "includes stories" do + expect(stories).to include(old_story) expect(stories).to include(new_story) end end @@ -153,8 +151,8 @@ RSpec.describe Articles::Feed, type: :service do let!(:new_story) { create(:article, published_at: 10.minutes.ago, score: 10) } let(:stories) { feed.mix_default_and_more_random } - it "includes stories from between 2 and 6 hours ago" do - expect(stories).not_to include(old_story) + it "includes stories" do + expect(stories).to include(old_story) expect(stories).to include(new_story) end end @@ -163,8 +161,8 @@ RSpec.describe Articles::Feed, type: :service do let!(:new_story) { create(:article, published_at: 10.minutes.ago, score: 10) } let(:stories) { feed.more_tag_weight } - it "includes stories from between 2 and 6 hours ago" do - expect(stories).not_to include(old_story) + it "includes stories" do + expect(stories).to include(old_story) expect(stories).to include(new_story) end end @@ -173,8 +171,8 @@ RSpec.describe Articles::Feed, type: :service do let!(:new_story) { create(:article, published_at: 10.minutes.ago, score: 10) } let(:stories) { feed.more_tag_weight_more_random } - it "includes stories from between 2 and 6 hours ago" do - expect(stories).not_to include(old_story) + it "includes stories" do + expect(stories).to include(old_story) expect(stories).to include(new_story) end end @@ -375,6 +373,52 @@ RSpec.describe Articles::Feed, type: :service do end end + describe ".globally_hot_articles" do + let!(:recently_published_article) { create(:article, published_at: 3.hours.ago) } + let(:globally_hot_articles) { feed.globally_hot_articles(true).second } + + it "returns hot stories" do + expect(globally_hot_articles).not_to be_empty + end + + it "returns recent stories" do + expect(globally_hot_articles).to include(recently_published_article) + end + + context "when low number of hot stories and no recently published articles" do + before do + Article.delete_all + create(:article, hotness_score: 1000, score: 1000, published_at: 3.hours.ago) + end + + # This test handles a situation in which there are a low number of hot or new stories, and the user is logged in. + # Previously the offest factor could result in zero stories being returned sometimes. + + # We manually called `feed.globally_hot_articles` here because `let` caches it! + it "still returns articles" do + empty_feed = false + 20.times do + if feed.globally_hot_articles(true).second.empty? + empty_feed = true + break + end + end + expect(empty_feed).to be false + end + end + + context "when now hot stories and no recently published articles" do + before do + Article.delete_all + create(:article, hotness_score: 0, score: 0, published_at: 3.days.ago) + end + + it "still returns articles" do + expect(globally_hot_articles).not_to be_empty + end + end + end + describe ".find_featured_story" do let(:featured_story) { described_class.find_featured_story(stories) }