diff --git a/app/controllers/stories/feeds_controller.rb b/app/controllers/stories/feeds_controller.rb index cac63353a..db66f1805 100644 --- a/app/controllers/stories/feeds_controller.rb +++ b/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 diff --git a/app/services/articles/feeds.rb b/app/services/articles/feeds.rb new file mode 100644 index 000000000..4f8e298b8 --- /dev/null +++ b/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 diff --git a/app/services/articles/feeds/large_forem_experimental.rb b/app/services/articles/feeds/large_forem_experimental.rb index a346739d1..f9b683276 100644 --- a/app/services/articles/feeds/large_forem_experimental.rb +++ b/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) diff --git a/app/services/articles/feeds/weighted_query_strategy.rb b/app/services/articles/feeds/weighted_query_strategy.rb index ed3848437..ccd8e3714 100644 --- a/app/services/articles/feeds/weighted_query_strategy.rb +++ b/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] :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 diff --git a/spec/requests/stories/feeds_spec.rb b/spec/requests/stories/feeds_spec.rb index 46837a313..00097c80f 100644 --- a/spec/requests/stories/feeds_spec.rb +++ b/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 diff --git a/spec/services/articles/feeds_spec.rb b/spec/services/articles/feeds_spec.rb new file mode 100644 index 000000000..fddaeda93 --- /dev/null +++ b/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