From fbce226c083054f50fd3bb035319e397ad3b235f Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Tue, 26 Oct 2021 09:57:33 -0400 Subject: [PATCH] Low Stakes Refactor of Feed class (#15180) * Removing state change and unused method Prior to this commit, the `@comment_weight` value would change. This is not ideal as depending on the call sequence, can notably change the output. I suspect this state change occurred so as to not alter an underlying spec. What this change does is remove the state change, removes a dead method, renames a method (to the dead method name), and leverages parameterization to better test a spec that was brittle based on possible state changes. I believe, from a logical stand point, that this change does not impact the functionality nor the actual logic that is part of the production call path. * Marking methods as @api private The goal is to highlight that we really shouldn't be calling these outside of their contained class. Ideally, I'd love to make them private methods, but there are specs and would prefer to not use `__send__` to change those specs. This is a noop change. * Renaming method to refelct returned param order Prior to this commit the "default_home_feed_and_featured_story" returned an array of `[featured_story, default_home_feed]`. The method name and the order of the returned values were misaligned. This change helps align the method name and the order of those returned values. Note, methods such as `each_with_index` have an `element, index` parameter order. --- app/controllers/stories/feeds_controller.rb | 2 +- app/controllers/stories_controller.rb | 2 +- .../feeds/large_forem_experimental.rb | 33 ++++++++++++------- .../feeds/large_forem_experimental_spec.rb | 22 ++++++------- 4 files changed, 35 insertions(+), 24 deletions(-) diff --git a/app/controllers/stories/feeds_controller.rb b/app/controllers/stories/feeds_controller.rb index efab40ee6..ba4f49817 100644 --- a/app/controllers/stories/feeds_controller.rb +++ b/app/controllers/stories/feeds_controller.rb @@ -38,7 +38,7 @@ module Stories Articles::Feeds::Basic.new(user: current_user, page: @page, tag: params[:tag]).feed else feed = Articles::Feeds::LargeForemExperimental.new(user: current_user, page: @page, tag: params[:tag]) - feed.more_comments_minimal_weight_randomized_at_end + feed.more_comments_minimal_weight_randomized end end diff --git a/app/controllers/stories_controller.rb b/app/controllers/stories_controller.rb index ab7b70eaa..cf1b7355f 100644 --- a/app/controllers/stories_controller.rb +++ b/app/controllers/stories_controller.rb @@ -242,7 +242,7 @@ class StoriesController < ApplicationController 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?) + @featured_story, @stories = feed.featured_story_and_default_home_feed(user_signed_in: user_signed_in?) end @pinned_article = pinned_article&.decorate diff --git a/app/services/articles/feeds/large_forem_experimental.rb b/app/services/articles/feeds/large_forem_experimental.rb index 54444002e..0ee5b8e79 100644 --- a/app/services/articles/feeds/large_forem_experimental.rb +++ b/app/services/articles/feeds/large_forem_experimental.rb @@ -7,32 +7,36 @@ module Articles @page = page @tag = tag @tag_weight = 1 # default weight tags play in rankings - @comment_weight = 0 # default weight comments play in rankings + @comment_weight = 0.2 # default weight comments play in rankings @xp_level_weight = 1 # default weight for user experience level 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) + _featured_story, stories = featured_story_and_default_home_feed(user_signed_in: user_signed_in, ranking: true) stories end - def default_home_feed_and_featured_story(user_signed_in: false, ranking: true) + def featured_story_and_default_home_feed(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 [featured_story, hot_stories] end - def more_comments_minimal_weight - @comment_weight = 0.2 - _featured_story, stories = default_home_feed_and_featured_story(user_signed_in: true) - stories + # Adding an alias to preserve public method signature. + # Eventually, we should be able to remove the alias. + alias default_home_feed_and_featured_story featured_story_and_default_home_feed + + def more_comments_minimal_weight_randomized + _featured_story, stories = featured_story_and_default_home_feed(user_signed_in: true) + first_quarter(stories).shuffle + last_three_quarters(stories) end - def more_comments_minimal_weight_randomized_at_end - results = more_comments_minimal_weight - first_quarter(results).shuffle + last_three_quarters(results) - end + # Adding an alias to preserve public method signature. However, + # in this code base there are no further references of + # :more_comments_minimal_weight_randomized_at_end + alias more_comments_minimal_weight_randomized_at_end more_comments_minimal_weight_randomized + # @api private def rank_and_sort_articles(articles) ranked_articles = articles.each_with_object({}) do |article, result| article_points = score_single_article(article) @@ -42,6 +46,7 @@ module Articles ranked_articles.to(@number_of_articles - 1) end + # @api private def score_single_article(article, base_article_points: 0) article_points = base_article_points article_points += score_followed_user(article) @@ -52,10 +57,12 @@ module Articles article_points end + # @api private def score_followed_user(article, follow_user_score: 1, not_followed_user_score: 0) user_following_users_ids.include?(article.user_id) ? follow_user_score : not_followed_user_score end + # @api private def score_followed_tags(article, nil_user_tag_score: 0, followed_tag_weight: @tag_weight, unfollowed_tag_score: 0) return nil_user_tag_score unless @user @@ -65,19 +72,23 @@ module Articles end end + # @api private def score_followed_organization(article, followed_org_score: 1, not_followed_org_score: 0) user_following_org_ids.include?(article.organization_id) ? followed_org_score : not_followed_org_score end + # @api private def score_experience_level(article, xp_level_weight: @xp_level_weight, default_user_xp_level: 5) user_experience_level = @user&.setting&.experience_level || default_user_xp_level - (((article.experience_level_rating - user_experience_level).abs / 2) * xp_level_weight) end + # @api private def score_comments(article, comment_weight: @comment_weight) article.comments_count * comment_weight end + # @api private def globally_hot_articles(user_signed_in, article_score_threshold: -15, min_rand_limit: 15, max_rand_limit: 80) if user_signed_in hot_stories = experimental_hot_story_grab diff --git a/spec/services/articles/feeds/large_forem_experimental_spec.rb b/spec/services/articles/feeds/large_forem_experimental_spec.rb index 39b1b7f7f..ebe8edcb6 100644 --- a/spec/services/articles/feeds/large_forem_experimental_spec.rb +++ b/spec/services/articles/feeds/large_forem_experimental_spec.rb @@ -11,8 +11,8 @@ RSpec.describe Articles::Feeds::LargeForemExperimental, type: :service do let!(:old_story) { create(:article, published_at: 3.days.ago) } let!(:low_scoring_article) { create(:article, score: -1000) } - describe "#default_home_feed_and_featured_story" do - let(:default_feed) { feed.default_home_feed_and_featured_story } + describe "#featured_story_and_default_home_feed" do + let(:default_feed) { feed.featured_story_and_default_home_feed } let(:featured_story) { default_feed.first } let(:stories) { default_feed.second } let!(:min_score_article) { create(:article, score: 0) } @@ -30,7 +30,7 @@ RSpec.describe Articles::Feeds::LargeForemExperimental, type: :service do end context "when user logged in" do - let(:result) { feed.default_home_feed_and_featured_story(user_signed_in: true) } + let(:result) { feed.featured_story_and_default_home_feed(user_signed_in: true) } let(:featured_story) { result.first } let(:stories) { result.second } @@ -59,7 +59,7 @@ RSpec.describe Articles::Feeds::LargeForemExperimental, type: :service do context "when ranking is true" do it "performs article ranking" do allow(feed).to receive(:rank_and_sort_articles).and_call_original - feed.default_home_feed_and_featured_story(ranking: true) + feed.featured_story_and_default_home_feed(ranking: true) expect(feed).to have_received(:rank_and_sort_articles) end end @@ -67,7 +67,7 @@ RSpec.describe Articles::Feeds::LargeForemExperimental, type: :service do context "when ranking is false" do it "does not perform article ranking" do allow(feed).to receive(:rank_and_sort_articles).and_call_original - feed.default_home_feed_and_featured_story(ranking: false) + feed.featured_story_and_default_home_feed(ranking: false) expect(feed).not_to have_received(:rank_and_sort_articles) end end @@ -75,7 +75,7 @@ RSpec.describe Articles::Feeds::LargeForemExperimental, type: :service do context "when ranking not passed" do it "performs article ranking" do allow(feed).to receive(:rank_and_sort_articles).and_call_original - feed.default_home_feed_and_featured_story + feed.featured_story_and_default_home_feed expect(feed).to have_received(:rank_and_sort_articles) end end @@ -123,10 +123,10 @@ RSpec.describe Articles::Feeds::LargeForemExperimental, type: :service do end end - describe "more_comments_minimal_weight_randomized_at_end" do + describe "more_comments_minimal_weight_randomized" do it "returns articles" do new_story = create(:article, published_at: 10.minutes.ago, score: 10) - stories = feed.more_comments_minimal_weight_randomized_at_end + stories = feed.more_comments_minimal_weight_randomized expect(stories).to include(old_story) expect(stories).to include(new_story) end @@ -298,12 +298,12 @@ RSpec.describe Articles::Feeds::LargeForemExperimental, type: :service do context "when comment_weight is default of 0" do it "returns 0 for uncommented articles" do - expect(feed.score_comments(article)).to eq(0) + expect(feed.score_comments(article, comment_weight: 1)).to eq(0) end - it "returns 0 for articles with comments" do + it "returns a multiple of the parameterized weight for articles with comments" do expect(article_with_five_comments.comments_count).to eq(5) - expect(feed.score_comments(article_with_five_comments)).to eq(0) + expect(feed.score_comments(article_with_five_comments, comment_weight: 1)).to eq(5) end end