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

@@ -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
 

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

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

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