Fixing weighted query hotness grab logic (#15528)

* Fixing weighted query hotness grab logic

Prior to this commit, I carried over (albeit imprecisely) logic from the
LargeForemExperimental.  That logic was to help limit articles to those
that were published since the user's latest page view.

However, I introduced a bug in this transcription.  Now both
LargeForemExperimental and WeightedQueryStrategy use the same logic to
determine the oldest publication date to search for in the feed.

This resolves a bug reported where users were not seeing a large number
of items in their feed.

Incidentally, if a forem has little activity in the 18 hours, there
might be very few items in the feed.

I believe, going forward, we may need to better parameterize how many
hours is considered "stale since last page view".

Related to #15240

* Fixing implementation detail

* Extracting helper method

Prior to this commit, I had introduced a method an put it in a less
ideal module space.  This commit extracts that method to a more readily
shareable module space.

I've added a few more specs to help clarify and verify behavior.

* Updating documentation

* Renaming and documenting variables/constants

* Fixing that which I broke

app/controllers/stories/feeds_controller.rb

@@ -3,6 +3,7 @@ module Stories
     respond_to :json
 
     def show
+      @page = (params[:page] || 1).to_i
       @stories = assign_feed_stories
 
       add_pinned_article

app/services/articles/feeds.rb

@@ -0,0 +1,49 @@
+module Articles
+  module Feeds
+    # The default number of days old that an article can be for us
+    # to consider it in the relevance feed.
+    #
+    # @note I believe that it is likely we would extract this constant
+    #       into an adminsitrative setting.  Hence, I want to keep it
+    #       a scalar.
+    DEFAULT_DAYS_SINCE_PUBLISHED = 7
+
+    # @note I believe that it is likely we would extract this constant
+    #       into an administrative setting.  Hence, I want to keep it
+    #       a scalar to ease the implementation details of the admin
+    #       setting.
+    NUMBER_OF_HOURS_TO_OFFSET_USERS_LATEST_ARTICLE_VIEWS = 18
+
+    # @api private
+    #
+    # This method helps answer the question: What are the articles
+    # that I should consider as new for the given user?  This method
+    # provides a date by which to filter out "stale to the user"
+    # articles.
+    #
+    # @note Do we need to continue using this method?  It's part of
+    #       the hot story grab experiment that we ran with the
+    #       Article::Feeds::LargeForemExperimental, but may not be
+    #       relevant.
+    #
+    # @param user [User]
+    # @param days_since_published [Integer] if someone
+    #        hasn't viewed any articles, give them things from the
+    #        database seeds.
+    #
+    # @return [ActiveSupport::TimeWithZone]
+    #
+    # @note the days_since_published is something carried
+    #       over from the LargeForemExperimental and may not be
+    #       relevant given that we have the :daily_factor_decay.
+    #       However, this further limitation based on a user's
+    #       second most recent page view helps further winnow down
+    #       the result set.
+    def self.oldest_published_at_to_consider_for(user:, days_since_published: DEFAULT_DAYS_SINCE_PUBLISHED)
+      time_of_second_latest_page_view = user&.page_views&.second_to_last&.created_at
+      return days_since_published.days.ago unless time_of_second_latest_page_view
+
+      time_of_second_latest_page_view - NUMBER_OF_HOURS_TO_OFFSET_USERS_LATEST_ARTICLE_VIEWS.hours
+    end
+  end
+end

app/services/articles/feeds/large_forem_experimental.rb

@@ -101,7 +101,7 @@ module Articles
       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
+        start_time = Articles::Feeds.oldest_published_at_to_consider_for(user: @user)
         Article.published.limited_column_select.includes(top_comments: :user)
           .where("published_at > ?", start_time)
           .page(@page).per(@number_of_articles)

app/services/articles/feeds/weighted_query_strategy.rb

@@ -46,31 +46,34 @@ module Articles
       #
       # Each scoring method has the following keys:
       #
-      # - clause: The SQL clause statement; note: there exists a
-      #           coupling between the clause and the SQL fragments
-      #           that join the various tables.  Also, under no
-      #           circumstances should you allow any user value for
+      # - clause: [Required] The SQL clause statement; note: there
+      #           exists a coupling between the clause and the SQL
+      #           fragments that join the various tables.  Also, under
+      #           no circumstances should you allow any user value for
       #           this, as it is not something we can sanitize.
       #
-      # - cases: An Array of Arrays, the first value is what matches
-      #          the clause, the second value is the multiplicative
-      #          factor.
+      # - cases: [Required] An Array of Arrays, the first value is
+      #          what matches the clause, the second value is the
+      #          multiplicative factor.
       #
-      # - fallback: When no case is matched use this factor.
+      # - fallback: [Required] When no case is matched use this
+      #             factor.
       #
-      # - requires_user: Does this scoring method require a given
-      #                  user.  If not, don't use it if we don't have
-      #                  a nil user.
+      # - requires_user: [Required] Does this scoring method require a
+      #                  given user.  If not, don't use it if we don't
+      #                  have a nil user.
       #
-      # - group_by: An SQL fragment that ensures a valid postgres
-      #             statement in older versions of postgres.  See
+      # - group_by: [Optional] An SQL fragment that ensures a valid
+      #             postgres statement in older versions of postgres.
+      #             See
       #             https://github.com/forem/forem/pull/15240#discussion_r750392321
       #             for further sleuthing details.  When you reference
       #             a field in the clause, you likely need to include
       #             a corresponding :group_by attribute.
       #
-      # - joins: An SQL fragment that defines the join necessary to
-      #          fulfill the clause of the scoring method.
+      # - joins: [Optional] An SQL fragment that defines the join
+      #          necessary to fulfill the clause of the scoring
+      #          method.
       #
       # @note The group by clause appears necessary for postgres
       #       versions and Heroku configurations of current (as of
@@ -243,8 +246,6 @@ module Articles
       # @option config [Array<Symbol>] :scoring_configs
       #   allows for you to configure which methods you want to use.
       #   This is most relevant when running A/B testing.
-      # @option config [Integer] :most_number_of_days_to_consider
-      #   defines the oldest published date that we'll consider.
       # @option config [Integer] :negative_reaction_threshold, when
       #         the `articles.privileged_users_reaction_points_sum` is
       #         less than this amount, treat this is a negative
@@ -259,18 +260,19 @@ module Articles
       def initialize(user: nil, number_of_articles: 50, page: 1, tag: nil, **config)
         @user = user
         @number_of_articles = number_of_articles.to_i
-        @page = page.to_i
+        @page = (page || 1).to_i
         # TODO: The tag parameter is vestigial, there's no logic around this value.
         @tag = tag
         @default_user_experience_level = config.fetch(:default_user_experience_level) { DEFAULT_USER_EXPERIENCE_LEVEL }
         @negative_reaction_threshold = config.fetch(:negative_reaction_threshold, DEFAULT_NEGATIVE_REACTION_THRESHOLD)
         @positive_reaction_threshold = config.fetch(:positive_reaction_threshold, DEFAULT_POSITIVE_REACTION_THRESHOLD)
-        @oldest_published_at = determine_oldest_published_at(
-          user: @user,
-          most_number_of_days_to_consider: config.fetch(:most_number_of_days_to_consider, 31),
-        )
         @scoring_configs = config.fetch(:scoring_configs) { default_scoring_configs }
         configure!(scoring_configs: @scoring_configs)
+
+        @oldest_published_at = Articles::Feeds.oldest_published_at_to_consider_for(
+          user: @user,
+          days_since_published: @days_since_published,
+        )
       end
 
       # The goal of this query is to generate a list of articles that
@@ -499,10 +501,6 @@ module Articles
         @joins.join("\n")
       end
 
-      def determine_oldest_published_at(user:, most_number_of_days_to_consider: 31)
-        user&.page_views&.second_to_last&.created_at || most_number_of_days_to_consider.days.ago
-      end
-
       # We multiply the relevance score components together.
       def relevance_score_components_as_sql
         @relevance_score_components.join(" * \n")
@@ -531,6 +529,7 @@ module Articles
       # @see SCORING_METHOD_CONFIGURATIONS
       # @note Be mindful to guard against SQL injection!
       def configure!(scoring_configs:)
+        @days_since_published = Articles::Feeds::DEFAULT_DAYS_SINCE_PUBLISHED
         @relevance_score_components = []
 
         # By default we always need to group by the articles.id
@@ -580,6 +579,12 @@ module Articles
             cases: scoring_config.fetch(:cases),
             fallback: scoring_config.fetch(:fallback),
           )
+
+          # Make sure that we consider all of the days for which we're
+          # establishing cases and for which there is a fallback.
+          if valid_method_name == :daily_decay_factor
+            @days_since_published = scoring_config.fetch(:cases).count + 1
+          end
         end
       end
 

spec/requests/stories/feeds_spec.rb

@@ -135,7 +135,7 @@ RSpec.describe "Stories::Feeds", type: :request do
 
         get timeframe_stories_feed_path(:week)
 
-        expect(Articles::Feeds::Timeframe).to have_received(:call).with("week", page: nil, tag: nil)
+        expect(Articles::Feeds::Timeframe).to have_received(:call).with("week", page: 1, tag: nil)
       end
 
       it "calls the feed service for latest" do

spec/services/articles/feeds_spec.rb

@@ -0,0 +1,44 @@
+require "rails_helper"
+
+RSpec.describe Articles::Feeds do
+  describe ".oldest_published_at_to_consider_for" do
+    subject(:function_call) { described_class.oldest_published_at_to_consider_for(user: user) }
+
+    context "when the given user is nil" do
+      let(:user) { nil }
+
+      it { is_expected.to be_a ActiveSupport::TimeWithZone }
+    end
+
+    context "when the given user has no page views" do
+      let(:user) { instance_double(User) }
+
+      before do
+        allow(user).to receive(:page_views).and_return(nil)
+      end
+
+      it { is_expected.to be_a ActiveSupport::TimeWithZone }
+    end
+
+    context "when the user has page views" do
+      let(:user) { instance_double(User) }
+      let(:page_viewed_at) { Time.current }
+      let(:expected_result) do
+        page_viewed_at - described_class::NUMBER_OF_HOURS_TO_OFFSET_USERS_LATEST_ARTICLE_VIEWS.hours
+      end
+
+      before do
+        # This is a distinct code smell.  The other option is adding a
+        # method to user and perhaps to page views and then testing
+        # delegation.  This is, instead, an ActiveRecord call chain,
+        # so I'm going to ask that we accept this smell to ease
+        # testing.
+        # rubocop:disable RSpec/MessageChain
+        allow(user).to receive_message_chain(:page_views, :second_to_last, :created_at).and_return(page_viewed_at)
+        # rubocop:enable RSpec/MessageChain
+      end
+
+      it { is_expected.to eq(expected_result) }
+    end
+  end
+end