From 7ba0b49ea8ee7b64481ef55351f847574018a846 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Thu, 18 Nov 2021 16:28:58 -0500 Subject: [PATCH] Parameterizing featured story requiring main image (#15333) * Parameterizing featured story requiring main image This begins to ease the resolution of #15292. If we merge #15240, I could see setting an `Article` constant for this value and allowing the administrator to choose the particular behavior. Once we have insight from the product team, we can move forward with a more comprehensive solution. * Adding default parameter While this is an `@api private` method, I am aligning the defaults with it's `@api public` caller. Yes, only specs call the method, but I'd rather not fiddle with the specs at this moment in time. * Reworking logic to be more scannable --- .../articles/feeds/find_featured_story.rb | 19 +++++++++++--- .../feeds/large_forem_experimental.rb | 25 +++++++++++++++---- .../feeds/find_featured_story_spec.rb | 14 +++++++++++ 3 files changed, 49 insertions(+), 9 deletions(-) diff --git a/app/services/articles/feeds/find_featured_story.rb b/app/services/articles/feeds/find_featured_story.rb index 1b7c4dba0..85158786f 100644 --- a/app/services/articles/feeds/find_featured_story.rb +++ b/app/services/articles/feeds/find_featured_story.rb @@ -1,12 +1,23 @@ module Articles module Feeds module FindFeaturedStory - def self.call(stories) + # @param stories [ActiveRecord::Relation, #detect] pick a + # featured story from this enumerable + # @param must_have_main_image [Boolean] if true, the featured + # story must have a main image + # @return [Article] + # + # @note the must_have_main_image parameter name matches PR #15240 + def self.call(stories, must_have_main_image: true) featured_story = - if stories.is_a?(ActiveRecord::Relation) - stories.where.not(main_image: nil).first + if must_have_main_image + if stories.is_a?(ActiveRecord::Relation) + stories.where.not(main_image: nil).first + else + stories.detect { |story| story.main_image.present? } + end else - stories.detect { |story| story.main_image.present? } + stories.first end featured_story || Article.new diff --git a/app/services/articles/feeds/large_forem_experimental.rb b/app/services/articles/feeds/large_forem_experimental.rb index 93694834e..a346739d1 100644 --- a/app/services/articles/feeds/large_forem_experimental.rb +++ b/app/services/articles/feeds/large_forem_experimental.rb @@ -14,8 +14,15 @@ module Articles stories end - def featured_story_and_default_home_feed(user_signed_in: false, ranking: true) - featured_story, hot_stories = globally_hot_articles(user_signed_in) + # @param user_signed_in [Boolean] are we treating this as an + # anonymous user? + # @param ranking [Boolean] if true, apply a ranking algorithm + # @param must_have_main_image [Boolean] if true, the featured + # story must have a main image + # + # @note the must_have_main_image parameter name matches PR #15240 + def featured_story_and_default_home_feed(user_signed_in: false, ranking: true, must_have_main_image: true) + featured_story, hot_stories = globally_hot_articles(user_signed_in, must_have_main_image: must_have_main_image) hot_stories = rank_and_sort_articles(hot_stories) if @user && ranking [featured_story, hot_stories] end @@ -63,11 +70,13 @@ module Articles to: :@article_score_applicator) # @api private - def globally_hot_articles(user_signed_in, article_score_threshold: -15, min_rand_limit: 15, max_rand_limit: 80) + # rubocop:disable Layout/LineLength + def globally_hot_articles(user_signed_in, must_have_main_image: true, article_score_threshold: -15, min_rand_limit: 15, max_rand_limit: 80) + # rubocop:enable Layout/LineLength if user_signed_in hot_stories = experimental_hot_story_grab hot_stories = hot_stories.where.not(user_id: UserBlock.cached_blocked_ids_for_blocker(@user.id)) - featured_story = hot_stories.where.not(main_image: nil).first + featured_story = featured_story_from(stories: hot_stories, must_have_main_image: must_have_main_image) new_stories = Article.published .where("score > ?", article_score_threshold) .limited_column_select.includes(top_comments: :user).order(published_at: :desc) @@ -78,13 +87,19 @@ module Articles .page(@page).per(@number_of_articles) .where("score >= ? OR featured = ?", Settings::UserExperience.home_feed_minimum_score, true) .order(hotness_score: :desc) - featured_story = hot_stories.where.not(main_image: nil).first + featured_story = featured_story_from(stories: hot_stories, must_have_main_image: must_have_main_image) end [featured_story, hot_stories.to_a] end private + def featured_story_from(stories:, must_have_main_image:) + return stories.first unless must_have_main_image + + stories.where.not(main_image: nil).first + end + def experimental_hot_story_grab start_time = [(@user.page_views.second_to_last&.created_at || 7.days.ago) - 18.hours, 7.days.ago].max Article.published.limited_column_select.includes(top_comments: :user) diff --git a/spec/services/articles/feeds/find_featured_story_spec.rb b/spec/services/articles/feeds/find_featured_story_spec.rb index aee65b6af..277fe39d3 100644 --- a/spec/services/articles/feeds/find_featured_story_spec.rb +++ b/spec/services/articles/feeds/find_featured_story_spec.rb @@ -8,6 +8,13 @@ RSpec.describe Articles::Feeds::FindFeaturedStory, type: :service do featured_story = described_class.call(Article.all) expect(featured_story.main_image).not_to be_nil end + + context "when must_have_main_image is false" do + it "returns the first article" do + featured_story = described_class.call(Article.all) + expect(featured_story).to be_persisted + end + end end context "when passed an array" do @@ -15,6 +22,13 @@ RSpec.describe Articles::Feeds::FindFeaturedStory, type: :service do featured_story = described_class.call(Article.all.to_a) expect(featured_story.main_image).to be_present end + + context "when must_have_main_image is false" do + it "returns the first article" do + featured_story = described_class.call(Article.all.to_a) + expect(featured_story).to be_persisted + end + end end context "when passed collection without any articles" do