maverick/docbrown
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Refactoring so relevancy levers define expected variables (#17591)

Browse source 
Prior to this commit, the SQL fragments included variables that were set
configured at a global level.

With this commit, we're now saying that each lever "knows" what variable
it needs; and providing the means at lever declaration time to "say"
what those variable names are.  (e.g. `Articles::Feeds::LEVER_CATALOG`).

Then as part of the variant configuration (in the
`./config/feed-variants/*.json` files) we now include the expected value
of those parameters; which by convention (and coercion) are integers.

This relates to forem/forem#17584 because we want to move from a
privileged user reaction that has two values (`negative` and `positive`)
into four values (`very_negative`, `negative`, `positive`, and
`very_positive`).  To do that, we'll create a new lever; but that's for
another pull request.
This commit is contained in:
Jeremy Friesen 2022-05-06 13:54:33 -04:00 • committed by GitHub
parent d897dedf6b
commit 38ee9ef309
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
9 changed files with 89 additions and 43 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
app/models/articles/feeds.rb
View file

@ -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.",

46
app/models/articles/feeds/relevancy_lever.rb
View file

@ -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<Symbol>] 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<Array<Integer, Float>>]
# @param fallback [Float]
# @param query_parameters [Hash<Symbol,Integer>] 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

5
app/models/articles/feeds/variant_assembler.rb
View file

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

30
app/services/articles/feeds/variant_query.rb
View file

@ -33,9 +33,6 @@ module Articles
:levers, # Array <Articles::Feeds::RelevancyLever::Configured>
: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

4
config/feed-variants/20220415-incumbent.json
View file

@ -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": [

4
config/feed-variants/20220422-jennie-variant.json
View file

@ -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": [

10
config/feed-variants/README.md
View file

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

4
config/feed-variants/original.json
View file

@ -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": [

23
spec/models/articles/feeds/relevancy_lever_spec.rb
View file

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