From c7ba9d81db508a7afa50bc7c4e0a8f1eac8be54d Mon Sep 17 00:00:00 2001 From: Michael Kohl Date: Fri, 17 Sep 2021 09:23:32 +0700 Subject: [PATCH] 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 | 2 +- app/controllers/stories/feeds_controller.rb | 16 +++----- .../stories/tagged_articles_controller.rb | 4 +- app/controllers/stories_controller.rb | 7 ++-- .../feeds/large_forem_experimental.rb | 32 --------------- app/services/articles/feeds/latest.rb | 15 +++++++ app/services/articles/feeds/tag.rb | 25 ++++++++++++ app/services/articles/feeds/timeframe.rb | 15 +++++++ spec/initializers/rack/attack_spec.rb | 4 +- spec/requests/articles/articles_spec.rb | 2 +- spec/requests/stories/feeds_spec.rb | 8 ++-- .../feeds/large_forem_experimental_spec.rb | 40 ------------------- spec/services/articles/feeds/latest_spec.rb | 19 +++++++++ spec/services/articles/feeds/tag_spec.rb | 20 ++++++++++ .../services/articles/feeds/timeframe_spec.rb | 17 ++++++++ 15 files changed, 127 insertions(+), 99 deletions(-) create mode 100644 app/services/articles/feeds/latest.rb create mode 100644 app/services/articles/feeds/tag.rb create mode 100644 app/services/articles/feeds/timeframe.rb create mode 100644 spec/services/articles/feeds/latest_spec.rb create mode 100644 spec/services/articles/feeds/tag_spec.rb create mode 100644 spec/services/articles/feeds/timeframe_spec.rb diff --git a/app/controllers/articles_controller.rb b/app/controllers/articles_controller.rb index f2c491352..f22384620 100644 --- a/app/controllers/articles_controller.rb +++ b/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 diff --git a/app/controllers/stories/feeds_controller.rb b/app/controllers/stories/feeds_controller.rb index ec29f87f1..efab40ee6 100644 --- a/app/controllers/stories/feeds_controller.rb +++ b/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 diff --git a/app/controllers/stories/tagged_articles_controller.rb b/app/controllers/stories/tagged_articles_controller.rb index 57831ba32..bbe0fdb16 100644 --- a/app/controllers/stories/tagged_articles_controller.rb +++ b/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 diff --git a/app/controllers/stories_controller.rb b/app/controllers/stories_controller.rb index 0ba2145dd..b35b8cb55 100644 --- a/app/controllers/stories_controller.rb +++ b/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 diff --git a/app/services/articles/feeds/large_forem_experimental.rb b/app/services/articles/feeds/large_forem_experimental.rb index 8ec685959..f8bf96beb 100644 --- a/app/services/articles/feeds/large_forem_experimental.rb +++ b/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 diff --git a/app/services/articles/feeds/latest.rb b/app/services/articles/feeds/latest.rb new file mode 100644 index 000000000..e6908df31 --- /dev/null +++ b/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 diff --git a/app/services/articles/feeds/tag.rb b/app/services/articles/feeds/tag.rb new file mode 100644 index 000000000..f3751a395 --- /dev/null +++ b/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 diff --git a/app/services/articles/feeds/timeframe.rb b/app/services/articles/feeds/timeframe.rb new file mode 100644 index 000000000..944b04e8e --- /dev/null +++ b/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 diff --git a/spec/initializers/rack/attack_spec.rb b/spec/initializers/rack/attack_spec.rb index 2877fa9f7..100e9e335 100644 --- a/spec/initializers/rack/attack_spec.rb +++ b/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 diff --git a/spec/requests/articles/articles_spec.rb b/spec/requests/articles/articles_spec.rb index 2bdd5a55c..81a1f8fe0 100644 --- a/spec/requests/articles/articles_spec.rb +++ b/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" } diff --git a/spec/requests/stories/feeds_spec.rb b/spec/requests/stories/feeds_spec.rb index ca3d51efb..95ab43f2b 100644 --- a/spec/requests/stories/feeds_spec.rb +++ b/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 diff --git a/spec/services/articles/feeds/large_forem_experimental_spec.rb b/spec/services/articles/feeds/large_forem_experimental_spec.rb index 238b2ef2c..48bf4bff7 100644 --- a/spec/services/articles/feeds/large_forem_experimental_spec.rb +++ b/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 } diff --git a/spec/services/articles/feeds/latest_spec.rb b/spec/services/articles/feeds/latest_spec.rb new file mode 100644 index 000000000..ce94aee35 --- /dev/null +++ b/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 diff --git a/spec/services/articles/feeds/tag_spec.rb b/spec/services/articles/feeds/tag_spec.rb new file mode 100644 index 000000000..b275460d6 --- /dev/null +++ b/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 diff --git a/spec/services/articles/feeds/timeframe_spec.rb b/spec/services/articles/feeds/timeframe_spec.rb new file mode 100644 index 000000000..af91a7b74 --- /dev/null +++ b/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