diff --git a/app/models/articles/feeds.rb b/app/models/articles/feeds.rb index 857d1695d..9798ab8b9 100644 --- a/app/models/articles/feeds.rb +++ b/app/models/articles/feeds.rb @@ -132,7 +132,8 @@ module Articles WHEN experience_level IS NULL THEN :default_user_experience_level ELSE experience_level END ) AS user_experience_level FROM users_settings WHERE users_settings.user_id = :user_id)))", - group_by_fragment: "articles.experience_level_rating") + group_by_fragment: "articles.experience_level_rating", + query_parameter_names: [:default_user_experience_level]) relevancy_lever(:featured_article, label: "Weight to give for feature or unfeatured articles. 1 is featured.", @@ -256,7 +257,8 @@ module Articles WHEN articles.privileged_users_reaction_points_sum < :negative_reaction_threshold THEN -1 WHEN articles.privileged_users_reaction_points_sum > :positive_reaction_threshold THEN 1 ELSE 0 END)", - group_by_fragment: "articles.privileged_users_reaction_points_sum") + group_by_fragment: "articles.privileged_users_reaction_points_sum", + query_parameter_names: %i[negative_reaction_threshold positive_reaction_threshold]) relevancy_lever(:public_reactions, label: "Weight to give for the number of unicorn, heart, reading list reactions for article.", diff --git a/app/models/articles/feeds/relevancy_lever.rb b/app/models/articles/feeds/relevancy_lever.rb index 564217714..8f2714537 100644 --- a/app/models/articles/feeds/relevancy_lever.rb +++ b/app/models/articles/feeds/relevancy_lever.rb @@ -20,6 +20,14 @@ module Articles end end + class InvalidQueryParametersError < ConfigurationError + def initialize(given_parameters:, expected_parameters:, key:) + # rubocop:disable Layout/LineLength + super("Expected query parameters #{expected_parameters.inspect}, got #{given_parameters.inspect} for lever #{key.inspect}") + # rubocop:enable Layout/LineLength + end + end + Configured = Struct.new( :key, :user_required, @@ -28,10 +36,12 @@ module Articles :group_by_fragment, :cases, :fallback, + :query_parameters, keyword_init: true, ) do alias_method :user_required?, :user_required end + # @param key [Symbol] the programmatic means of naming this # lever. (e.g. "publication_date_decay_lever") # @param label [String] the the "help text" for describing this lever. (e.g. "How the @@ -44,8 +54,11 @@ module Articles # given :select_fragment can properly query the database. # @param group_by_fragment [String] a SQL `GROUP BY` fragment used to ensure the given # :select_fragment can properly query the database. + # @param query_parameter_names [Array] The names of variables needed for the SQL + # fragments. + # # rubocop:disable Layout/LineLength - def initialize(key:, label:, range:, user_required:, select_fragment:, joins_fragments: [], group_by_fragment: nil) + def initialize(key:, label:, range:, user_required:, select_fragment:, joins_fragments: [], group_by_fragment: nil, query_parameter_names: []) @key = key.to_sym @label = label @range = range @@ -53,10 +66,12 @@ module Articles @select_fragment = select_fragment @joins_fragments = Array.wrap(joins_fragments) @group_by_fragment = group_by_fragment + @query_parameter_names = Array.wrap(query_parameter_names).map(&:to_sym) end # rubocop:enable Layout/LineLength - attr_reader :key, :label, :user_required, :select_fragment, :joins_fragments, :group_by_fragment + attr_reader :key, :label, :user_required, :select_fragment, :joins_fragments, :group_by_fragment, + :query_parameter_names alias user_required? user_required @@ -64,15 +79,21 @@ module Articles # # @param cases [Array>] # @param fallback [Float] + # @param query_parameters [Hash] A Hash of the named query parameter and it's + # corresponding value. # # @return [Articles::Feeds::RelevancyLever::Configured] # @raise [Articles::Feeds::RelevancyLever::InvalidFallbackError] when the given fallback is # invalid. - # @raise [Articles::Feeds::RelevancyLever::InvaidCasesError] when the given cases is invalid. - def configure_with(cases:, fallback:) + # @raise [Articles::Feeds::RelevancyLever::InvalidCasesError] when the given cases is invalid. + # @raise [Articles::Feeds::RelevancyLever::InvalidQueryParametersError] when the given query + # parameters are mismatched. + def configure_with(cases:, fallback:, **query_parameters) raise InvalidFallbackError.new(fallback: fallback, key: key) unless valid_fallback?(fallback) raise InvalidCasesError.new(cases: cases, key: key) unless valid_cases?(cases) + query_parameters = extract_query_parameters(query_parameters) + Configured.new( key: key, user_required: user_required, @@ -81,11 +102,28 @@ module Articles group_by_fragment: group_by_fragment, cases: cases, fallback: fallback, + query_parameters: query_parameters, ) end private + def extract_query_parameters(query_parameters) + returning_value = {} + query_parameter_names.each do |name| + # Cast to string because the given query_parameters is almost certainly from JSON and has + # a string key. + returning_value[name] = query_parameters.fetch(name).to_i + end + returning_value + rescue KeyError + raise InvalidQueryParametersError.new( + given_parameters: query_parameters.keys.map(&:to_sym), + expected_parameters: query_parameter_names, + key: key, + ) + end + def valid_fallback?(fallback) fallback.is_a?(Numeric) end diff --git a/app/models/articles/feeds/variant_assembler.rb b/app/models/articles/feeds/variant_assembler.rb index 29b127e2c..2f52f2c46 100644 --- a/app/models/articles/feeds/variant_assembler.rb +++ b/app/models/articles/feeds/variant_assembler.rb @@ -48,7 +48,7 @@ module Articles def self.build_with(catalog:, config:, variant:) relevancy_levers = config.fetch("levers").map do |key, settings| lever = catalog.fetch_lever(key) - lever.configure_with(cases: settings.fetch("cases"), fallback: settings.fetch("fallback")) + lever.configure_with(**settings.symbolize_keys) end VariantQuery::Config.new( @@ -56,9 +56,6 @@ module Articles levers: relevancy_levers, order_by: catalog.fetch_order_by(config.fetch("order_by")), max_days_since_published: config.fetch("max_days_since_published"), - 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), ) end private_class_method :build_with diff --git a/app/services/articles/feeds/variant_query.rb b/app/services/articles/feeds/variant_query.rb index c380052c4..d2af5b2e3 100644 --- a/app/services/articles/feeds/variant_query.rb +++ b/app/services/articles/feeds/variant_query.rb @@ -33,9 +33,6 @@ module Articles :levers, # Array :order_by, # Articles::Feeds::OrderByLever :max_days_since_published, - :default_user_experience_level, - :negative_reaction_threshold, - :positive_reaction_threshold, keyword_init: true, ) @@ -50,23 +47,15 @@ module Articles @page = page @tag = tag @config = config - @oldest_published_at = Articles::Feeds.oldest_published_at_to_consider_for( + oldest_published_at = Articles::Feeds.oldest_published_at_to_consider_for( user: @user, - days_since_published: max_days_since_published, + days_since_published: config.max_days_since_published, ) - + @query_parameters = { oldest_published_at: oldest_published_at } configure! end - attr_reader :oldest_published_at, :config - - delegate( - :max_days_since_published, - :negative_reaction_threshold, - :positive_reaction_threshold, - :default_user_experience_level, - to: :config, - ) + attr_reader :config, :query_parameters # Query for articles relevant to the user's interest. # @@ -94,15 +83,11 @@ module Articles # rubocop:enable Layout/LineLength # These are the variables we'll pass to the SQL statement. - repeated_query_variables = { - negative_reaction_threshold: negative_reaction_threshold, - positive_reaction_threshold: positive_reaction_threshold, - oldest_published_at: oldest_published_at, + repeated_query_variables = query_parameters.merge( omit_article_ids: omit_article_ids, now: Time.current, user_id: @user&.id, - default_user_experience_level: default_user_experience_level.to_i - } + ) # This needs to be an Array for Article.sanitize_sql. unsanitized_sql_sub_query = [ @@ -220,6 +205,9 @@ module Articles cases: lever.cases, fallback: lever.fallback, ) + + # As implemented, this is not looking for collisions of named parameters. + @query_parameters.merge!(lever.query_parameters) end end diff --git a/config/feed-variants/20220415-incumbent.json b/config/feed-variants/20220415-incumbent.json index 178ccb178..0226a0c6c 100644 --- a/config/feed-variants/20220415-incumbent.json +++ b/config/feed-variants/20220415-incumbent.json @@ -87,7 +87,9 @@ [-1, 0.2], [1, 1] ], - "fallback": 0.95 + "fallback": 0.95, + "negative_reaction_threshold": -10, + "positive_reaction_threshold": 10 }, "public_reactions": { "cases": [ diff --git a/config/feed-variants/20220422-jennie-variant.json b/config/feed-variants/20220422-jennie-variant.json index b522e5eb9..695c6347d 100644 --- a/config/feed-variants/20220422-jennie-variant.json +++ b/config/feed-variants/20220422-jennie-variant.json @@ -92,7 +92,9 @@ [-1, 0.2], [1, 1] ], - "fallback": 0.95 + "fallback": 0.95, + "negative_reaction_threshold": -10, + "positive_reaction_threshold": 10 }, "public_reactions": { "cases": [ diff --git a/config/feed-variants/README.md b/config/feed-variants/README.md index 47da8e183..2142d081a 100644 --- a/config/feed-variants/README.md +++ b/config/feed-variants/README.md @@ -36,15 +36,7 @@ omitted for that variant (but available for other variants). The available sort order is also defined in `Articles::Feeds::LEVER_CATALOG`. The high level parameters are defined in `Aritcles::Feeds::VariantQuery::Config` -and as of <2022-04-20 Wed> are: +and as of <2022-05-06 Fri> are: - **_max_days_since_published_:** only consider articles that were published no more than the _max_days_since_published_. -- **_default_user_experience_level_:** what do we consider to be the default - user experience level. -- **_negative_reaction_threshold_:** if an article has less than this value for - it's trusted/privileged user reaction points, consider it a low quality - article. -- **_positive_reaction_threshold_:** if an article has more than this value for - it's trusted/privileged user reaction points, consider it a low quality - article. diff --git a/config/feed-variants/original.json b/config/feed-variants/original.json index 178ccb178..0226a0c6c 100644 --- a/config/feed-variants/original.json +++ b/config/feed-variants/original.json @@ -87,7 +87,9 @@ [-1, 0.2], [1, 1] ], - "fallback": 0.95 + "fallback": 0.95, + "negative_reaction_threshold": -10, + "positive_reaction_threshold": 10 }, "public_reactions": { "cases": [ diff --git a/spec/models/articles/feeds/relevancy_lever_spec.rb b/spec/models/articles/feeds/relevancy_lever_spec.rb index 2fdad7e2c..0c4b555b9 100644 --- a/spec/models/articles/feeds/relevancy_lever_spec.rb +++ b/spec/models/articles/feeds/relevancy_lever_spec.rb @@ -44,5 +44,28 @@ RSpec.describe Articles::Feeds::RelevancyLever do it { within_block_is_expected.to raise_error described_class::InvalidCasesError } end + + context "when lever has query parameters" do + let(:lever) do + described_class.new( + key: :my_key, + range: "[0..10)", + label: "My label", + select_fragment: "articles.reaction_count", + user_required: true, + query_parameter_names: [:threshold], + ) + end + + it "sets the configured query parameters" do + configuration = lever.configure_with(fallback: fallback, cases: cases, threshold: 1) + expect(configuration.query_parameters).to eq({ threshold: 1 }) + end + + it "raises InvalidQueryParametersError when not provided" do + expect { lever.configure_with(fallback: fallback, cases: cases) } + .to raise_error(described_class::InvalidQueryParametersError) + end + end end end