Clean up feed-related code (#14707)

* Start cleaning up feed code

* Move published_articles_by_tag to service

* Add timeframe and latest feed services

* fixup! Move published_articles_by_tag to service

* Move constant to correct location

* Fix spec

app/controllers/articles_controller.rb

@@ -28,7 +28,7 @@ class ArticlesController < ApplicationController
                   handle_tag_feed
                 elsif request.path == latest_feed_path
                   @articles
-                    .where("score > ?", Articles::Feeds::LargeForemExperimental::MINIMUM_SCORE_LATEST_FEED)
+                    .where("score > ?", Articles::Feeds::Latest::MINIMUM_SCORE)
                     .includes(:user)
                 else
                   @articles

app/controllers/stories/feeds_controller.rb

@@ -37,7 +37,8 @@ module Stories
       if Settings::UserExperience.feed_strategy == "basic"
         Articles::Feeds::Basic.new(user: current_user, page: @page, tag: params[:tag]).feed
       else
-        optimized_signed_in_feed
+        feed = Articles::Feeds::LargeForemExperimental.new(user: current_user, page: @page, tag: params[:tag])
+        feed.more_comments_minimal_weight_randomized_at_end
       end
     end
 
@@ -46,23 +47,16 @@ module Stories
         Articles::Feeds::Basic.new(user: nil, page: @page, tag: params[:tag]).feed
       else
         Articles::Feeds::LargeForemExperimental.new(user: current_user, page: @page, tag: params[:tag])
-          .default_home_feed(user_signed_in: user_signed_in?)
+          .default_home_feed(user_signed_in: false)
       end
     end
 
     def timeframe_feed
-      feed = Articles::Feeds::LargeForemExperimental.new(user: current_user, page: @page, tag: params[:tag])
-      feed.top_articles_by_timeframe(timeframe: params[:timeframe])
+      Articles::Feeds::Timeframe.call(params[:timeframe], tag: params[:tag], page: @page)
     end
 
     def latest_feed
-      feed = Articles::Feeds::LargeForemExperimental.new(user: current_user, page: @page, tag: params[:tag])
-      feed.latest_feed
-    end
-
-    def optimized_signed_in_feed
-      feed = Articles::Feeds::LargeForemExperimental.new(user: current_user, page: @page, tag: params[:tag])
-      feed.more_comments_minimal_weight_randomized_at_end
+      Articles::Feeds::Latest.call(tag: params[:tag], page: @page)
     end
   end
 end

app/controllers/stories/tagged_articles_controller.rb

@@ -44,9 +44,7 @@ module Stories
     end
 
     def set_stories
-      @stories = Articles::Feeds::LargeForemExperimental
-        .new(number_of_articles: @number_of_articles, tag: @tag, page: @page)
-        .published_articles_by_tag
+      @stories = Articles::Feeds::Tag.call(@tag, number_of_articles: @number_of_articles, page: @page)
 
       @stories = @stories.where(approved: true) if @tag_model&.requires_approval
 

app/controllers/stories_controller.rb

@@ -235,14 +235,13 @@ class StoriesController < ApplicationController
   end
 
   def assign_feed_stories
-    feed = Articles::Feeds::LargeForemExperimental.new(page: @page, tag: params[:tag])
-
     if params[:timeframe].in?(Timeframe::FILTER_TIMEFRAMES)
-      @stories = feed.top_articles_by_timeframe(timeframe: params[:timeframe])
+      @stories = Articles::Feeds::Timeframe.call(params[:timeframe])
     elsif params[:timeframe] == Timeframe::LATEST_TIMEFRAME
-      @stories = feed.latest_feed
+      @stories = Articles::Feeds::Latest.call
     else
       @default_home_feed = true
+      feed = Articles::Feeds::LargeForemExperimental.new(page: @page, tag: params[:tag])
       @featured_story, @stories = feed.default_home_feed_and_featured_story(user_signed_in: user_signed_in?)
     end
 

app/services/articles/feeds/large_forem_experimental.rb

@@ -1,9 +1,6 @@
 module Articles
   module Feeds
     class LargeForemExperimental
-      include FieldTest::Helpers
-      MINIMUM_SCORE_LATEST_FEED = -20
-
       def initialize(user: nil, number_of_articles: 50, page: 1, tag: nil)
         @user = user
         @number_of_articles = number_of_articles
@@ -27,40 +24,11 @@ module Articles
         self.class.find_featured_story(stories)
       end
 
-      def published_articles_by_tag
-        articles =
-          if @tag.present?
-            if FeatureFlag.enabled?(:optimize_article_tag_query)
-              Article.cached_tagged_with_any(@tag)
-            else
-              Tag.find_by(name: @tag).articles
-            end
-          else
-            Article.all
-          end
-
-        articles.published.limited_column_select
-          .includes(top_comments: :user)
-          .page(@page).per(@number_of_articles)
-      end
-
-      # Timeframe values from Timeframe::DATETIMES
-      def top_articles_by_timeframe(timeframe:)
-        published_articles_by_tag.where("published_at > ?", Timeframe.datetime(timeframe))
-          .order(score: :desc).page(@page).per(@number_of_articles)
-      end
-
       def default_home_feed(user_signed_in: false)
         _featured_story, stories = default_home_feed_and_featured_story(user_signed_in: user_signed_in, ranking: true)
         stories
       end
 
-      def latest_feed
-        published_articles_by_tag.order(published_at: :desc)
-          .where("score > ?", MINIMUM_SCORE_LATEST_FEED)
-          .page(@page).per(@number_of_articles)
-      end
-
       def default_home_feed_and_featured_story(user_signed_in: false, ranking: true)
         featured_story, hot_stories = globally_hot_articles(user_signed_in)
         hot_stories = rank_and_sort_articles(hot_stories) if @user && ranking

app/services/articles/feeds/latest.rb

@@ -0,0 +1,15 @@
+module Articles
+  module Feeds
+    module Latest
+      MINIMUM_SCORE = -20
+
+      def self.call(tag: nil, number_of_articles: 50, page: 1)
+        Articles::Feeds::Tag.call(tag)
+          .order(published_at: :desc)
+          .where("score > ?", MINIMUM_SCORE)
+          .page(page)
+          .per(number_of_articles)
+      end
+    end
+  end
+end

app/services/articles/feeds/tag.rb

@@ -0,0 +1,25 @@
+module Articles
+  module Feeds
+    module Tag
+      def self.call(tag = nil, number_of_articles: 50, page: 1)
+        articles =
+          if tag.present?
+            if FeatureFlag.enabled?(:optimize_article_tag_query)
+              Article.cached_tagged_with_any(tag)
+            else
+              ::Tag.find_by(name: tag).articles
+            end
+          else
+            Article.all
+          end
+
+        articles
+          .published
+          .limited_column_select
+          .includes(top_comments: :user)
+          .page(page)
+          .per(number_of_articles)
+      end
+    end
+  end
+end

app/services/articles/feeds/timeframe.rb

@@ -0,0 +1,15 @@
+module Articles
+  module Feeds
+    module Timeframe
+      def self.call(timeframe, tag: nil, number_of_articles: 50, page: 1)
+        articles = ::Articles::Feeds::Tag.call(tag)
+
+        articles
+          .where("published_at > ?", ::Timeframe.datetime(timeframe))
+          .order(score: :desc)
+          .page(page)
+          .per(number_of_articles)
+      end
+    end
+  end
+end

spec/initializers/rack/attack_spec.rb

@@ -136,9 +136,7 @@ describe Rack::Attack, type: :request, throttle: true do
       allow_any_instance_of(Stories::TaggedArticlesController).to receive(:tagged_count).and_return(0)
       allow_any_instance_of(Stories::TaggedArticlesController).to receive(:stories_by_timeframe)
         .and_return(Article.none)
-      allow_any_instance_of(Articles::Feeds::LargeForemExperimental).to receive(
-        :published_articles_by_tag,
-      ).and_return(Article.none)
+      allow(Articles::Feeds::Tag).to receive(:call).and_return(Article.none)
       tag_path = "/t/#{create(:tag).name}"
 
       get tag_path, headers: headers # warm up the slow endpoint

spec/requests/articles/articles_spec.rb

@@ -190,7 +190,7 @@ RSpec.describe "Articles", type: :request do
     let!(:last_article) { create(:article, featured: true) }
     let!(:not_featured_article) { create(:article, featured: false) }
     let!(:article_with_low_score) do
-      create(:article, score: Articles::Feeds::LargeForemExperimental::MINIMUM_SCORE_LATEST_FEED)
+      create(:article, score: Articles::Feeds::Latest::MINIMUM_SCORE)
     end
 
     before { get "/feed/latest" }

spec/requests/stories/feeds_spec.rb

@@ -131,20 +131,20 @@ RSpec.describe "Stories::Feeds", type: :request do
 
       it "calls the feed service for a timeframe" do
         allow(Articles::Feeds::LargeForemExperimental).to receive(:new).and_return(feed_service)
-        allow(feed_service).to receive(:top_articles_by_timeframe).with(timeframe: "week").and_call_original
+        allow(Articles::Feeds::Timeframe).to receive(:call).and_call_original
 
         get timeframe_stories_feed_path(:week)
 
-        expect(feed_service).to have_received(:top_articles_by_timeframe).with(timeframe: "week")
+        expect(Articles::Feeds::Timeframe).to have_received(:call).with("week", page: nil, tag: nil)
       end
 
       it "calls the feed service for latest" do
         allow(Articles::Feeds::LargeForemExperimental).to receive(:new).and_return(feed_service)
-        allow(feed_service).to receive(:latest_feed).and_call_original
+        allow(Articles::Feeds::Latest).to receive(:call).and_call_original
 
         get timeframe_stories_feed_path(:latest)
 
-        expect(feed_service).to have_received(:latest_feed)
+        expect(Articles::Feeds::Latest).to have_received(:call)
       end
     end
 

spec/services/articles/feeds/large_forem_experimental_spec.rb

@@ -10,46 +10,6 @@ RSpec.describe Articles::Feeds::LargeForemExperimental, type: :service do
   end
   let!(:old_story) { create(:article, published_at: 3.days.ago) }
   let!(:low_scoring_article) { create(:article, score: -1000) }
-  let!(:month_old_story) { create(:article, published_at: 1.month.ago) }
-
-  describe "#published_articles_by_tag" do
-    let(:unpublished_article) { create(:article, published: false) }
-    let(:tag) { "foo" }
-    let!(:tagged_article) { create(:article, tags: tag) }
-
-    it "returns published articles" do
-      result = feed.published_articles_by_tag
-      expect(result).to include article
-      expect(result).not_to include unpublished_article
-    end
-
-    context "with tag" do
-      it "returns articles with the specified tag" do
-        expect(described_class.new(tag: tag).published_articles_by_tag).to include tagged_article
-      end
-    end
-  end
-
-  describe "#top_articles_by_timeframe" do
-    let!(:moderately_high_scoring_article) { create(:article, score: 20) }
-    let(:result) { feed.top_articles_by_timeframe(timeframe: "week").to_a }
-
-    it "returns correct articles ordered by score" do
-      expect(result.slice(0, 2)).to eq [hot_story, moderately_high_scoring_article]
-      expect(result.last).to eq low_scoring_article
-      expect(result).not_to include(month_old_story)
-    end
-  end
-
-  describe "#latest_feed" do
-    it "only returns articles with scores above -40" do
-      expect(feed.latest_feed).not_to include(low_scoring_article)
-    end
-
-    it "returns articles ordered by publishing date descending" do
-      expect(feed.latest_feed.last).to eq month_old_story
-    end
-  end
 
   describe "#default_home_feed_and_featured_story" do
     let(:default_feed) { feed.default_home_feed_and_featured_story }

spec/services/articles/feeds/latest_spec.rb

@@ -0,0 +1,19 @@
+require "rails_helper"
+
+RSpec.describe Articles::Feeds::Latest, type: :service do
+  let!(:hot_article) do
+    create(:article, hotness_score: 1000, score: 1000, published_at: 3.hours.ago, user: create(:user))
+  end
+  let!(:newest_article) { create(:article, published_at: 1.hour.ago) }
+  let!(:month_old_article) { create(:article, published_at: 1.month.ago) }
+  let!(:low_scoring_article) { create(:article, score: -1000) }
+
+  it "returns articles ordered by publishing date descending" do
+    result = described_class.call
+    expect(result).to eq [newest_article, hot_article, month_old_article]
+  end
+
+  it "only returns articles with scores above the minumum" do
+    expect(described_class.call).not_to include(low_scoring_article)
+  end
+end

spec/services/articles/feeds/tag_spec.rb

@@ -0,0 +1,20 @@
+require "rails_helper"
+
+RSpec.describe Articles::Feeds::Tag, type: :service do
+  let(:tag) { "tag" }
+  let!(:article) { create(:article) }
+  let!(:tagged_article) { create(:article, tags: tag) }
+  let!(:unpublished_article) { create(:article, published: false) }
+
+  it "returns published articles only" do
+    result = described_class.call
+    expect(result).to include article
+    expect(result).not_to include unpublished_article
+  end
+
+  context "with tag" do
+    it "returns articles with the specified tag" do
+      expect(described_class.call(tag)).to include tagged_article
+    end
+  end
+end

spec/services/articles/feeds/timeframe_spec.rb

@@ -0,0 +1,17 @@
+require "rails_helper"
+
+RSpec.describe Articles::Feeds::Timeframe, type: :service do
+  let!(:hot_article) do
+    create(:article, hotness_score: 1000, score: 1000, published_at: 3.hours.ago, user: create(:user))
+  end
+  let!(:low_scoring_article) { create(:article, score: -1000) }
+  let!(:moderately_high_scoring_article) { create(:article, score: 20) }
+  let!(:month_old_article) { create(:article, published_at: 1.month.ago) }
+
+  it "returns correct articles ordered by score", :aggregate_failures do
+    result = described_class.call("week")
+    expect(result.slice(0, 2)).to eq [hot_article, moderately_high_scoring_article]
+    expect(result.last).to eq low_scoring_article
+    expect(result).not_to include(month_old_article)
+  end
+end