From 0d0464be2fceecfb214fb76b28c05b1c5420c6b0 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Thu, 21 Apr 2022 11:07:09 -0400 Subject: [PATCH] Allowing VariantQuery for Feed Generation (#17382) * Allowing VariantQuery for Feed Generation Apologies for the breadth of this pull request, I had considered many small commits, but felt that would've been more effort for the value provided. This commit includes the following: - Documentation updates to the feed variant (though not the final pass) - Renaming and adding RelevancyLevers that help differentiate - Adding RelevancyLever#range to provide documentation - Reducing redundent controller logic by making a `Articles::Feeds.feed_for` method. - Adding some configuration validation for RelevancyLevers - Adding constants for better clarification - Testing unhappy paths for feed configuration - Adjusting the module namespace of some objects - Exposing top-level configurations for variants (along with their defaults) - Creating the VariantyQuery that at present inherits from the `Articles::Feeds::WeightedQueryStrategy` As implemented, we can deploy this code to production without using the new VariantQuery. Once we toggle on the `:feed_uses_variant_query_feature` FeatureFlag, it will switch to using the VariantQuery. The VariantQuery's two variants and the internal configuration of `Articles::Feeds::WeightedQueryStrategy` produce the same query. The goal of this factor is to allow for a quick on and off toggle of the feed query; to ensure that what we introduce remains performant. - Closes forem/forem#17272 - Closes forem/forem#17276 - Closes forem/forem#17216 In addition, I will be recording a code-walkthrough and linking that recording to the pull request. * Apply suggestions from code review Co-authored-by: Mac Siri Co-authored-by: Mac Siri --- app/controllers/stories/feeds_controller.rb | 28 ++-- app/models/ab_experiment.rb | 22 ++- app/models/articles/feeds.rb | 131 ++++++++++++++++-- .../articles/feeds/lever_catalog_builder.rb | 15 +- app/models/articles/feeds/order_by_lever.rb | 6 +- app/models/articles/feeds/relevancy_lever.rb | 75 +++++++++- .../articles/feeds/variant_assembler.rb | 73 +++++----- app/services/articles/feeds/variant_query.rb | 113 +++++++++++++++ .../articles/feeds/weighted_query_strategy.rb | 30 ++-- config/feed-variants/20220415-incumbent.json | 4 +- config/feed-variants/README.md | 50 +++++++ config/feed-variants/original.json | 102 ++++++++++++++ .../feed-variants/broken/broken-fallback.json | 26 ++++ spec/models/ab_experiment_spec.rb | 13 ++ .../feeds/lever_catalog_builder_spec.rb | 22 ++- .../articles/feeds/order_by_lever_spec.rb | 1 + .../articles/feeds/relevancy_lever_spec.rb | 32 ++++- .../articles/feeds/variant_assembler_spec.rb | 10 +- spec/requests/stories/feeds_spec.rb | 30 ++-- .../articles/feeds/variant_query_spec.rb | 60 ++++++++ 20 files changed, 709 insertions(+), 134 deletions(-) create mode 100644 app/services/articles/feeds/variant_query.rb create mode 100644 config/feed-variants/README.md create mode 100644 config/feed-variants/original.json create mode 100644 spec/fixtures/feed-variants/broken/broken-fallback.json create mode 100644 spec/services/articles/feeds/variant_query_spec.rb diff --git a/app/controllers/stories/feeds_controller.rb b/app/controllers/stories/feeds_controller.rb index 84b7cfdb1..08b60a7e7 100644 --- a/app/controllers/stories/feeds_controller.rb +++ b/app/controllers/stories/feeds_controller.rb @@ -38,17 +38,12 @@ module Stories feed = if Settings::UserExperience.feed_strategy == "basic" Articles::Feeds::Basic.new(user: current_user, page: @page, tag: params[:tag]) else - strategy = AbExperiment.get( - experiment: AbExperiment::CURRENT_FEED_STRATEGY_EXPERIMENT, - controller: self, user: current_user, - default_value: AbExperiment::ORIGINAL_VARIANT - ) - Articles::Feeds::WeightedQueryStrategy.new( + Articles::Feeds.feed_for( user: current_user, - number_of_articles: 25, + controller: self, page: @page, - tags: params[:tag], - strategy: strategy, + tag: params[:tag], + number_of_articles: 25, ) end Datadog.tracer.trace("feed.query", @@ -65,15 +60,16 @@ module Stories end def signed_out_base_feed - strategy = AbExperiment.get(experiment: AbExperiment::CURRENT_FEED_STRATEGY_EXPERIMENT, controller: self, - user: current_user, default_value: AbExperiment::ORIGINAL_VARIANT) - feed = if strategy.weighted_query_strategy? - Articles::Feeds::WeightedQueryStrategy.new(user: current_user, page: @page, tags: params[:tag]) - elsif Settings::UserExperience.feed_strategy == "basic" - # I'm a bit uncertain why we're skipping the user on this call. + feed = if Settings::UserExperience.feed_strategy == "basic" Articles::Feeds::Basic.new(user: nil, page: @page, tag: params[:tag]) else - Articles::Feeds::LargeForemExperimental.new(user: current_user, page: @page, tag: params[:tag]) + Articles::Feeds.feed_for( + user: current_user, + controller: self, + page: @page, + tag: params[:tag], + number_of_articles: 25, + ) end Datadog.tracer.trace("feed.query", span_type: "db", diff --git a/app/models/ab_experiment.rb b/app/models/ab_experiment.rb index bee6a45a2..2072b02d2 100644 --- a/app/models/ab_experiment.rb +++ b/app/models/ab_experiment.rb @@ -11,10 +11,11 @@ class AbExperiment < SimpleDelegator # public private_class_method :new + # @see ./config/feed-variants/README.md ORIGINAL_VARIANT = "original".freeze CURRENT_FEED_STRATEGY_EXPERIMENT = FieldTest.config["experiments"]&.keys - &.detect { |e| e.start_with? "feed_strategy" }.freeze + &.detect { |e| e.start_with? "feed_strategy" }.freeze # Sometimes we might want to repurpose the same AbExperiment logic # for different experiments. This provides the tooling for that @@ -34,6 +35,23 @@ class AbExperiment < SimpleDelegator EXPERIMENT_TO_METHOD_NAME_MAP.fetch(experiment, experiment) end + # A convenience method for retrieving the feed variant + # @param controller [ApplicationController] the request context of + # the experiment. + # @param user [User] who are we running the experiment with + # + # @return [Object] a variant that the experimenter should know what it wants + # + # @see .get + def self.get_feed_variant_for(controller:, user:) + get( + controller: controller, + user: user, + default_value: ORIGINAL_VARIANT, + experiment: CURRENT_FEED_STRATEGY_EXPERIMENT, + ) + end + def self.variants_for_experiment(experiment) configured_experiment = FieldTest.config["experiments"][experiment] configured_experiment["variants"] if configured_experiment @@ -64,7 +82,7 @@ class AbExperiment < SimpleDelegator # on a configured set of values. They know which one they # likely want. Let them give us a hint. # - # @return [Object] the experimenter should know what it wants + # @return [Object] a variant that the experimenter should know what it wants # # @see config/field_test.yml file for configured experiments. # diff --git a/app/models/articles/feeds.rb b/app/models/articles/feeds.rb index bed5f151b..fb62f9e86 100644 --- a/app/models/articles/feeds.rb +++ b/app/models/articles/feeds.rb @@ -14,6 +14,10 @@ module Articles # setting. NUMBER_OF_HOURS_TO_OFFSET_USERS_LATEST_ARTICLE_VIEWS = 18 + DEFAULT_USER_EXPERIENCE_LEVEL = 5 + DEFAULT_NEGATIVE_REACTION_THRESHOLD = -10 + DEFAULT_POSITIVE_REACTION_THRESHOLD = 10 + # @api private # # This method helps answer the question: What are the articles @@ -35,7 +39,7 @@ module Articles # # @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. + # relevant given that we have the :daily_decay. # However, this further limitation based on a user's # second most recent page view helps further winnow down # the result set. @@ -46,6 +50,40 @@ module Articles time_of_second_latest_page_view - NUMBER_OF_HOURS_TO_OFFSET_USERS_LATEST_ARTICLE_VIEWS.hours end + # Get the properly configured feed for the given user (and other parameters). + # + # @param controller [ApplicationController] used to retrieve the field_test variant + # @param user [User, NilClass] used to retrieve the variant and how we query the articles + # @param number_of_articles [Integer] the pagination page size + # @param page [Integer] the page on which to start pagination + # @param tag [NilClass, String] not used but carried forward for interface conformance + # + # @return [Articles::Feeds::VariantQuery] if the :feed_uses_variant_query_feature feature is + # enabled. + # @return [Articles::Feeds::WeightedQueryStrategy] if the :feed_uses_variant_query_feature + # feature is not enabled. + def self.feed_for(controller:, user:, number_of_articles:, page:, tag:) + variant = AbExperiment.get_feed_variant_for(controller: controller, user: user) + + if FeatureFlag.enabled?(:feed_uses_variant_query_feature) + VariantQuery.build_for( + variant: variant, + user: user, + number_of_articles: number_of_articles, + page: page, + tag: tag, + ) + else + WeightedQueryStrategy.new( + variant: variant, + user: user, + number_of_articles: number_of_articles, + page: page, + tag: tag, + ) + end + end + # The available feed levers for this Forem instance. # # @return [Articles::Feeds::LeverCatalogBuilder] @@ -56,7 +94,7 @@ module Articles # rubocop:disable Metrics/BlockLength # The available levers for this forem instance. LEVER_CATALOG = LeverCatalogBuilder.new do - order_by_lever(OrderByLever::DEFAULT_KEY, + order_by_lever(:relevancy_score_and_publication_date, label: "Order by highest calculated relevancy score then latest published at time.", order_by_fragment: "article_relevancies.relevancy_score DESC, articles.published_at DESC") @@ -67,14 +105,15 @@ module Articles relevancy_lever(:comments_count_by_those_followed, label: "Weight to give for the number of comments on the article from other users" \ "that the given user follows.", + range: "[0..∞)", user_required: true, select_fragment: "COUNT(comments_by_followed.id)", - joins_fragment: ["LEFT OUTER JOIN follows AS followed_user + joins_fragments: ["LEFT OUTER JOIN follows AS followed_user ON articles.user_id = followed_user.followable_id AND followed_user.followable_type = 'User' AND followed_user.follower_id = :user_id AND followed_user.follower_type = 'User'", - "LEFT OUTER JOIN comments AS comments_by_followed + "LEFT OUTER JOIN comments AS comments_by_followed ON comments_by_followed.commentable_id = articles.id AND comments_by_followed.commentable_type = 'Article' AND followed_user.followable_id = comments_by_followed.user_id @@ -84,12 +123,14 @@ module Articles relevancy_lever(:comments_count, label: "Weight to give to the number of comments on the article.", + range: "[0..∞)", user_required: false, select_fragment: "articles.comments_count", group_by_fragment: "articles.comments_count") relevancy_lever(:daily_decay, label: "Weight given based on the relative age of the article", + range: "[0..∞)", user_required: true, select_fragment: "(current_date - articles.published_at::date)", group_by_fragment: "articles.published_at") @@ -97,6 +138,7 @@ module Articles relevancy_lever(:experience, label: "Weight to give based on the difference between experience level of the " \ "article and given user.", + range: "[0..∞)", user_required: true, select_fragment: "ROUND(ABS(articles.experience_level_rating - (SELECT (CASE @@ -108,15 +150,17 @@ module Articles relevancy_lever(:featured_article, label: "Weight to give for feature or unfeatured articles. 1 is featured.", user_required: false, + range: "[0..1]", select_fragment: "(CASE articles.featured WHEN true THEN 1 ELSE 0 END)", group_by_fragment: "articles.featured") relevancy_lever(:following_author, label: "Weight to give when the given user follows the article's author." \ "1 is followed, 0 is not followed.", + range: "[0..1]", user_required: true, select_fragment: "COUNT(followed_user.follower_id)", - joins_fragment: ["LEFT OUTER JOIN follows AS followed_user + joins_fragments: ["LEFT OUTER JOIN follows AS followed_user ON articles.user_id = followed_user.followable_id AND followed_user.followable_type = 'User' AND followed_user.follower_id = :user_id @@ -125,9 +169,10 @@ module Articles relevancy_lever(:following_org, label: "Weight to give to the when the given user follows the article's organization." \ "1 is followed, 0 is not followed.", + range: "[0..1]", user_required: true, select_fragment: "COUNT(followed_org.follower_id)", - joins_fragment: ["LEFT OUTER JOIN follows AS followed_org + joins_fragments: ["LEFT OUTER JOIN follows AS followed_org ON articles.organization_id = followed_org.followable_id AND followed_org.followable_type = 'Organization' AND followed_org.follower_id = :user_id @@ -135,25 +180,81 @@ module Articles relevancy_lever(:latest_comment, label: "Weight to give an article based on it's most recent comment.", + range: "[0..∞)", user_required: false, select_fragment: "(current_date - MAX(comments.created_at)::date)", - joins_fragment: ["LEFT OUTER JOIN comments + joins_fragments: ["LEFT OUTER JOIN comments ON comments.commentable_id = articles.id AND comments.commentable_type = 'Article' AND comments.deleted = false AND comments.created_at > :oldest_published_at"]) - relevancy_lever(:matching_tags, - label: "Weight to give for the sum points of the intersecting tags of the article" \ - "user positive follows.", + relevancy_lever(:matching_negative_tag_intersection_count, + label: "Weight to give the number of intersecting tags of the article and " \ + "user negative follows", + range: "[0..4]", user_required: true, - select_fragment: "LEAST(10.0, SUM(followed_tags.points))::integer", - joins_fragment: ["LEFT OUTER JOIN taggings + select_fragment: "COUNT(negative_followed_tags.id)", + joins_fragments: ["LEFT OUTER JOIN taggings ON taggings.taggable_id = articles.id AND taggable_type = 'Article'", - "INNER JOIN tags + "INNER JOIN tags ON taggings.tag_id = tags.id", - "LEFT OUTER JOIN follows AS followed_tags + "LEFT OUTER JOIN follows AS negative_followed_tags + ON tags.id = negative_followed_tags.followable_id + AND negative_followed_tags.followable_type = 'ActsAsTaggableOn::Tag' + AND negative_followed_tags.follower_type = 'User' + AND negative_followed_tags.follower_id = :user_id + AND negative_followed_tags.explicit_points < 0"]) + + relevancy_lever(:matching_negative_tags_intersection_points, + label: "Weight to give for the sum points of the intersecting tags of the article and " \ + "user positive follows.", + user_required: true, + range: "[-10..0]", + select_fragment: "LEAST(-10.0, SUM(followed_tags.points))::integer", + joins_fragments: ["LEFT OUTER JOIN taggings + ON taggings.taggable_id = articles.id + AND taggable_type = 'Article'", + "INNER JOIN tags + ON taggings.tag_id = tags.id", + "LEFT OUTER JOIN follows AS followed_tags + ON tags.id = followed_tags.followable_id + AND followed_tags.followable_type = 'ActsAsTaggableOn::Tag' + AND followed_tags.follower_type = 'User' + AND followed_tags.follower_id = :user_id + AND followed_tags.explicit_points < 0"]) + + relevancy_lever(:matching_positive_tags_intersection_count, + label: "Weight to give for number of the intersecting tags of the article and " \ + "user positive follows.", + range: "[0..4]", + user_required: true, + select_fragment: "COUNT(followed_tags.id)", + joins_fragments: ["LEFT OUTER JOIN taggings + ON taggings.taggable_id = articles.id + AND taggable_type = 'Article'", + "INNER JOIN tags + ON taggings.tag_id = tags.id", + "LEFT OUTER JOIN follows AS followed_tags + ON tags.id = followed_tags.followable_id + AND followed_tags.followable_type = 'ActsAsTaggableOn::Tag' + AND followed_tags.follower_type = 'User' + AND followed_tags.follower_id = :user_id + AND followed_tags.explicit_points >= 0"]) + + relevancy_lever(:matching_positive_tags_intersection_points, + label: "Weight to give for the sum points of the intersecting tags of the article and " \ + "user positive follows.", + user_required: true, + range: "[0..10]", + select_fragment: "LEAST(10.0, SUM(followed_tags.points))::integer", + joins_fragments: ["LEFT OUTER JOIN taggings + ON taggings.taggable_id = articles.id + AND taggable_type = 'Article'", + "INNER JOIN tags + ON taggings.tag_id = tags.id", + "LEFT OUTER JOIN follows AS followed_tags ON tags.id = followed_tags.followable_id AND followed_tags.followable_type = 'ActsAsTaggableOn::Tag' AND followed_tags.follower_type = 'User' @@ -163,6 +264,7 @@ module Articles relevancy_lever(:privileged_user_reaction, label: "-1 when privileged user reactions down-vote, 0 when netural, and 1 when positive.", user_required: false, + range: "[-1..1]", select_fragment: "(CASE WHEN articles.privileged_users_reaction_points_sum < :negative_reaction_threshold THEN -1 WHEN articles.privileged_users_reaction_points_sum > :positive_reaction_threshold THEN 1 @@ -171,6 +273,7 @@ module Articles relevancy_lever(:public_reactions, label: "Weight to give for the number of unicorn, heart, reading list reactions for article.", + range: "[0..∞)", user_required: false, select_fragment: "articles.public_reactions_count", group_by_fragment: "articles.public_reactions_count") diff --git a/app/models/articles/feeds/lever_catalog_builder.rb b/app/models/articles/feeds/lever_catalog_builder.rb index 9051dfa4e..3e6ed8408 100644 --- a/app/models/articles/feeds/lever_catalog_builder.rb +++ b/app/models/articles/feeds/lever_catalog_builder.rb @@ -9,15 +9,10 @@ module Articles end end - class ConfigurationError < StandardError - end - # @yieldparam [Articles::Feeds::LeverCatalogBuilder] # # @raise [Articles::Feeds::LeverCatalogBuilder::DuplicateLeverError] when you attempt to # register a lever with the same key. - # @raise [Articles::Feeds::LeverCatalogBuilder::ConfigurationError] when you fail to configure - # the default order_by_lever # # @note Once initialized, this object and its constituent parts are frozen to prevent further # modification. In other words, once instantiated our published catalog has been @@ -30,8 +25,6 @@ module Articles # `#instance_exec` method. instance_exec(&config) - raise ConfigurationError unless @order_by_levers.key?(OrderByLever::DEFAULT_KEY) - @relevancy_levers.freeze @order_by_levers.freeze freeze @@ -46,17 +39,13 @@ module Articles @relevancy_levers.fetch(key.to_sym) end - # @param key [#to_sym, NilClass] when given nil fallback to the OrderByLever::DEFAULT_KEY. + # @param key [#to_sym] # @return [Articles::Feeds::OrderByLever] # # @raise [KeyError] if the given key is not found in the list of sort levers; in other words # we have a configuration mismatch. - # - # @see Articles::Feeds::OrderByLever::DEFAULT_KEY def fetch_order_by(key) - key ||= OrderByLever::DEFAULT_KEY - key = key.to_sym - @order_by_levers.fetch(key) + @order_by_levers.fetch(key.to_sym) end protected diff --git a/app/models/articles/feeds/order_by_lever.rb b/app/models/articles/feeds/order_by_lever.rb index a512a22be..d972982a6 100644 --- a/app/models/articles/feeds/order_by_lever.rb +++ b/app/models/articles/feeds/order_by_lever.rb @@ -5,8 +5,6 @@ module Articles # # @see config/feed/README.md class OrderByLever - DEFAULT_KEY = :relevancy_score_and_publication_date - # @param key [Symbol] the programmatic means of naming this # lever. (e.g. "publication_date_decay_lever") # @param label [String] the "help text" for describing this lever. (e.g. "How the @@ -18,6 +16,10 @@ module Articles @order_by_fragment = order_by_fragment end attr_reader :key, :label, :order_by_fragment + + def to_sql + Arel.sql(order_by_fragment) + end end end end diff --git a/app/models/articles/feeds/relevancy_lever.rb b/app/models/articles/feeds/relevancy_lever.rb index 517de5e89..564217714 100644 --- a/app/models/articles/feeds/relevancy_lever.rb +++ b/app/models/articles/feeds/relevancy_lever.rb @@ -5,29 +5,96 @@ module Articles # # @see config/feed/README.md class RelevancyLever + class ConfigurationError < StandardError + end + + class InvalidFallbackError < ConfigurationError + def initialize(fallback:, key:) + super("Expected fallback to be a Numeric value for lever #{key.inspect}, got #{fallback.inspect}") + end + end + + class InvalidCasesError < ConfigurationError + def initialize(cases:, key:) + super("Expected cases to be an array of number pairs for lever #{key.inspect}, got #{cases.inspect}") + end + end + + Configured = Struct.new( + :key, + :user_required, + :select_fragment, + :joins_fragments, + :group_by_fragment, + :cases, + :fallback, + 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 # publication date impacts relevancy score?") + # @param range [String] the expected range of the query results. # @param user_required [Boolean] if true, this lever is only available when we are building # the feed query for a given user. # @param select_fragment [String] a SQL `SELECT` fragment used to create the *lever range* - # @param joins_fragment [Array] an array of SQL `JOIN` fragments used to ensure the + # @param joins_fragments [Array] an array of SQL `JOIN` fragments used to ensure the # 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. - def initialize(key:, label:, user_required:, select_fragment:, joins_fragment: [], group_by_fragment: nil) + # rubocop:disable Layout/LineLength + def initialize(key:, label:, range:, user_required:, select_fragment:, joins_fragments: [], group_by_fragment: nil) @key = key.to_sym @label = label + @range = range @user_required = user_required @select_fragment = select_fragment - @joins_fragment = Array.wrap(joins_fragment) + @joins_fragments = Array.wrap(joins_fragments) @group_by_fragment = group_by_fragment end + # rubocop:enable Layout/LineLength - attr_reader :key, :label, :user_required, :select_fragment, :joins_fragment, :group_by_fragment + attr_reader :key, :label, :user_required, :select_fragment, :joins_fragments, :group_by_fragment alias user_required? user_required + + # Responsible for configuring the lever with the given input. + # + # @param cases [Array>] + # @param fallback [Float] + # + # @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 InvalidFallbackError.new(fallback: fallback, key: key) unless valid_fallback?(fallback) + raise InvalidCasesError.new(cases: cases, key: key) unless valid_cases?(cases) + + Configured.new( + key: key, + user_required: user_required, + select_fragment: select_fragment, + joins_fragments: joins_fragments, + group_by_fragment: group_by_fragment, + cases: cases, + fallback: fallback, + ) + end + + private + + def valid_fallback?(fallback) + fallback.is_a?(Numeric) + end + + def valid_cases?(cases) + return false unless cases.is_a?(Array) + + cases.all? { |range, factor| range.is_a?(Numeric) && factor.is_a?(Numeric) } + end end end end diff --git a/app/models/articles/feeds/variant_assembler.rb b/app/models/articles/feeds/variant_assembler.rb index fed262eea..29b127e2c 100644 --- a/app/models/articles/feeds/variant_assembler.rb +++ b/app/models/articles/feeds/variant_assembler.rb @@ -5,68 +5,63 @@ module Articles # # @see .call module VariantAssembler + # The Rails root relative path to the directory of feed variants + DIRECTORY = "config/feed-variants".freeze + + # The default extension for feed variants + EXTENSION = "json".freeze + # Assemble the named :variant based on the configuration of levers. # # @param variant [#to_sym,String,Symbol] the name of the variant we're assembling - # @param levers [Articles::Feeds::LeverCatalogBuilder] the available levers for assembly - # @param variants [Hash] the cache of previously assembled variants + # @param catalog [Articles::Feeds::LeverCatalogBuilder] the available levers for assembly + # @param variants [Hash] the cache of previously assembled variants; don't go rebuilding if we + # already have one. + # @param dir [String] the relative directory that contains the variants. # - # @return [Articles::Feeds::VariantQueryConfig] - def self.call(variant:, levers: Articles::Feeds.lever_catalog, variants: variants_cache) + # @raise [Errno::ENOENT] if named variant does not exist in DIRECTORY. In other + # words, we have a mismatch in configuration. + # + # @return [Articles::Feeds::VariantQuery::Config] + def self.call(variant:, catalog: Articles::Feeds.lever_catalog, variants: variants_cache, dir: DIRECTORY) variant = variant.to_sym variants[variant] ||= begin - content = Rails.root.join("config/feed-variants/#{variant}.json").read + content = Rails.root.join(dir, "#{variant}.#{EXTENSION}").read config = JSON.parse(content) - build_with(levers: levers, config: config, variant: variant) + build_with(catalog: catalog, config: config, variant: variant) end end + # @return [Hash] def self.variants_cache @variants_cache ||= {} end private_class_method :variants_cache - def self.build_with(levers:, config:, variant:) + # @param catalog [Articles::Feeds::LeverCatalogBuilder] + # @param variant [Symbol] + # @param config [Hash] + # + # @raise [KeyError] if we he have an invalidate configuration. + # + # @return [Articles::Feeds::VariantQuery::Config] + def self.build_with(catalog:, config:, variant:) relevancy_levers = config.fetch("levers").map do |key, settings| - lever = levers.fetch_lever(key) - ConfiguredLever.new( - key: lever.key, - user_required: lever.user_required, - select_fragment: lever.select_fragment, - joins_fragment: lever.joins_fragment, - group_by_fragment: lever.group_by_fragment, - cases: settings.fetch("cases"), - fallback: settings.fetch("fallback"), - ) + lever = catalog.fetch_lever(key) + lever.configure_with(cases: settings.fetch("cases"), fallback: settings.fetch("fallback")) end - VariantQueryConfig.new( + VariantQuery::Config.new( variant: variant, levers: relevancy_levers, - order_by: levers.fetch_order_by(config["order_by"]), + 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 end - - VariantQueryConfig = Struct.new( - :variant, - :levers, - :order_by, - keyword_init: true, - ) - - ConfiguredLever = Struct.new( - :key, - :user_required, - :select_fragment, - :joins_fragment, - :group_by_fragment, - :cases, - :fallback, - keyword_init: true, - ) do - alias_method :user_required?, :user_required - end end end diff --git a/app/services/articles/feeds/variant_query.rb b/app/services/articles/feeds/variant_query.rb new file mode 100644 index 000000000..e30eee946 --- /dev/null +++ b/app/services/articles/feeds/variant_query.rb @@ -0,0 +1,113 @@ +module Articles + module Feeds + # @note In the current implementation this is inheriting from WeightedQueryStrategy; going + # forward, we want to move away from the less flexible WeightedQueryStrategy. However, as + # we roll this out, we want both to be utilized. In part so we can have a quick fallback + # to a known working state (e.g. WeightedQueryStrategy). Assuming we move forward with + # this implementation, we will break the inheritance, copy the relevant methods over, and + # remove the WeightedQueryStrategy. + class VariantQuery < WeightedQueryStrategy + # @param variant [Symbol, #to_sym] the name of the variant query we're building. + # @param assembler [Articles::Feeds::VariantAssembler, #call] responsible for converting the + # given variant to a config suitable for building a VariantQuery. + # @param kwargs [Hash] named parameters to pass along to the #initialize method. + # + # @return [Articles::Feeds::VariantQuery] + # + # @see #initialize + def self.build_for(variant:, assembler: VariantAssembler, **kwargs) + config = assembler.call(variant: variant) + new(config: config, **kwargs) + end + + # Let's make sure that folks initialize this with a variant configuration. + private_class_method :new + + Config = Struct.new( + :variant, + :levers, # Array + :order_by, # Articles::Feeds::OrderByLever + :max_days_since_published, + :default_user_experience_level, + :negative_reaction_threshold, + :positive_reaction_threshold, + keyword_init: true, + ) + + # @param config [Articles::Feeds::VariantQuery::Config] + # @param user [User,NilClass] + # @param number_of_articles [Integer, #to_i] + # @param page [Integer, #to_i] + # @param tag [NilClass] not used + # + # rubocop:disable Lint/MissingSuper + def initialize(config:, user: nil, number_of_articles: 50, page: 1, tag: nil) + @user = user + @number_of_articles = number_of_articles + @page = page + @tag = tag + @config = config + @oldest_published_at = Articles::Feeds.oldest_published_at_to_consider_for( + user: @user, + days_since_published: max_days_since_published, + ) + + configure! + end + # rubocop:enable Lint/MissingSuper + + delegate( + :max_days_since_published, + :negative_reaction_threshold, + :positive_reaction_threshold, + :default_user_experience_level, + to: :config, + ) + + attr_reader :config + + private + + def final_order_logic(articles) + articles.order(config.order_by.to_sql) + end + + def configure! + @relevance_score_components = [] + + # By default we always need to group by the articles.id + # column. And as we add scoring methods to the query, we need + # to add additional group_by clauses based on the chosen + # scoring method. + @group_by_fields = Set.new + @group_by_fields << "articles.id" + + @joins = Set.new + + # Ensure that we honor a user's block requests. + unless @user.nil? + @joins << "LEFT OUTER JOIN user_blocks + ON user_blocks.blocked_id = articles.user_id + AND user_blocks.blocked_id IS NULL + AND user_blocks.blocker_id = :user_id" + end + + config.levers.each do |lever| + # Don't attempt to use this factor if we don't have user. + next if lever.user_required? && @user.nil? + + # This scoring method requires a group by clause. + @group_by_fields << lever.group_by_fragment if lever.group_by_fragment.present? + + @joins += lever.joins_fragments if lever.joins_fragments.present? + + @relevance_score_components << build_score_element_from( + clause: lever.select_fragment, + cases: lever.cases, + fallback: lever.fallback, + ) + end + end + end + end +end diff --git a/app/services/articles/feeds/weighted_query_strategy.rb b/app/services/articles/feeds/weighted_query_strategy.rb index 359c32cd4..70ac3685f 100644 --- a/app/services/articles/feeds/weighted_query_strategy.rb +++ b/app/services/articles/feeds/weighted_query_strategy.rb @@ -230,18 +230,13 @@ module Articles } }.freeze - DEFAULT_USER_EXPERIENCE_LEVEL = 5 - - DEFAULT_NEGATIVE_REACTION_THRESHOLD = -10 - DEFAULT_POSITIVE_REACTION_THRESHOLD = 10 - # @param user [User] who are we querying for? # @param number_of_articles [Integer] how many articles are we # returning # @param page [Integer] what is the pagination page # @param tag [String, nil] this isn't implemented in other feeds # so we'll see - # @param strategy [String, "original"] pass a current a/b test in + # @param variant [String, "original"] pass a current a/b test in # @param config [Hash] a list of configurations, # see {#initialize} implementation details. # @option config [Array] :scoring_configs @@ -260,13 +255,13 @@ module Articles # those will likely need some kind of structured consideration. # # rubocop:disable Layout/LineLength - def initialize(user: nil, number_of_articles: 50, page: 1, tag: nil, strategy: AbExperiment::ORIGINAL_VARIANT, **config) + def initialize(user: nil, number_of_articles: 50, page: 1, tag: nil, variant: AbExperiment::ORIGINAL_VARIANT, **config) @user = user @number_of_articles = number_of_articles.to_i @page = (page || 1).to_i # TODO: The tag parameter is vestigial, there's no logic around this value. @tag = tag - @strategy = strategy + @variant = variant @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) @@ -278,6 +273,8 @@ module Articles days_since_published: @days_since_published, ) end + attr_reader :oldest_published_at, :positive_reaction_threshold, :negative_reaction_threshold, :default_user_experience_level + # rubocop:enable Layout/LineLength # The goal of this query is to generate a list of articles that @@ -316,9 +313,9 @@ module Articles # rubocop:disable Layout/LineLength def call(only_featured: false, must_have_main_image: false, limit: default_limit, offset: default_offset, omit_article_ids: []) repeated_query_variables = { - negative_reaction_threshold: @negative_reaction_threshold, - positive_reaction_threshold: @positive_reaction_threshold, - oldest_published_at: @oldest_published_at, + negative_reaction_threshold: negative_reaction_threshold, + positive_reaction_threshold: positive_reaction_threshold, + oldest_published_at: oldest_published_at, omit_article_ids: omit_article_ids, now: Time.current } @@ -344,7 +341,7 @@ module Articles ), repeated_query_variables.merge({ user_id: @user.id, - default_user_experience_level: @default_user_experience_level.to_i + default_user_experience_level: default_user_experience_level.to_i }), ] end @@ -359,10 +356,11 @@ module Articles # can use to help ensure that we can use all of the # ActiveRecord goodness of scopes (e.g., # limited_column_select) and eager includes. - Article.joins(join_fragment) + + results = Article.joins(join_fragment) .limited_column_select .includes(top_comments: :user) - .order("article_relevancies.relevancy_score DESC, articles.published_at DESC") + final_order_logic(results) end # rubocop:enable Layout/LineLength @@ -438,6 +436,10 @@ module Articles private + def final_order_logic(scope) + scope.order("article_relevancies.relevancy_score DESC, articles.published_at DESC") + end + # Concatenate the required group by clauses. # # @return [String] diff --git a/config/feed-variants/20220415-incumbent.json b/config/feed-variants/20220415-incumbent.json index 9e1ff5d15..178ccb178 100644 --- a/config/feed-variants/20220415-incumbent.json +++ b/config/feed-variants/20220415-incumbent.json @@ -1,4 +1,6 @@ { + "max_days_since_published": 15, + "order_by": "relevancy_score_and_publication_date", "levers": { "daily_decay": { "cases": [ @@ -65,7 +67,7 @@ ], "fallback": 0.988 }, - "matching_tags": { + "matching_positive_tags_intersection_points": { "cases": [ [0, 0.7], [1, 0.7303], diff --git a/config/feed-variants/README.md b/config/feed-variants/README.md new file mode 100644 index 000000000..47da8e183 --- /dev/null +++ b/config/feed-variants/README.md @@ -0,0 +1,50 @@ +# Feed Variants + +This folder contains the feed variants that we have, are, or will be testing. By +convention the file's basename without extension is the name of the variant +(e.g. "./config/feed-variants/20220415-incumbent.json" encodes the variant named +`20220415-incumbent`). + +There exists one variant, `original.json`, which is our fallback variant. This +fallback is performant and adequate for generating the feed for DEV.to. It is +present in case we need to quickly turn off an experiment. The name `original` +is the same as the `AbExperiment::ORIGINAL_VARIANT` constant. + +**Do not remove `original.json` without consideration for how to rollback a feed +experiment.** + +Variants are loaded into production as needed. That "need" is determined by the +configuration of the `./config/field_test.yml`. Once we load a variant into +production, we cache that variant (see +`Articles::Feeds::VariantAssembler.variants_cache`). + +As part of our test suite we assemble and verify each defined variant. + +## How to Make a Feed Variant + +A feed variant is written in JSON format. A feed variant configures one or more +`Articles::Feeds::RelevancyLevers`. The variant configuration defines: + +- The cases and fallback for each of the selected relevancy levers +- Optionally the sort order of the query results +- Optionally a few high level parameters. + +The relevancy levers and their SQL query fragments are defined in +`Articles::Feeds::LEVER_CATALOG`. Any levers not configured for a variant are +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: + +- **_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 new file mode 100644 index 000000000..178ccb178 --- /dev/null +++ b/config/feed-variants/original.json @@ -0,0 +1,102 @@ +{ + "max_days_since_published": 15, + "order_by": "relevancy_score_and_publication_date", + "levers": { + "daily_decay": { + "cases": [ + [0, 1], + [1, 0.99], + [2, 0.985], + [3, 0.98], + [4, 0.975], + [5, 0.97], + [6, 0.965], + [7, 0.96], + [8, 0.955], + [9, 0.95], + [10, 0.945], + [11, 0.94], + [12, 0.935], + [13, 0.93], + [14, 0.925] + ], + "fallback": 0.9 + }, + "comments_count_by_those_followed": { + "cases": [ + [0, 0.95], + [1, 0.98], + [2, 0.99] + ], + "fallback": 0.93 + }, + "comments_count": { + "cases": [ + [0, 0.8], + [1, 0.82], + [2, 0.84], + [3, 0.86], + [4, 0.88], + [5, 0.9], + [6, 0.92], + [7, 0.94], + [8, 0.96], + [9, 0.98] + ], + "fallback": 1 + }, + "featured_article": { "cases": [[1, 1]], "fallback": 0.85 }, + "following_author": { + "cases": [ + [0, 0.8], + [1, 1] + ], + "fallback": 1 + }, + "following_org": { + "cases": [ + [0, 0.95], + [1, 1] + ], + "fallback": 1 + }, + "latest_comment": { + "cases": [ + [0, 1], + [1, 0.9988] + ], + "fallback": 0.988 + }, + "matching_positive_tags_intersection_points": { + "cases": [ + [0, 0.7], + [1, 0.7303], + [2, 0.7606], + [3, 0.7909], + [4, 0.8212], + [5, 0.8515], + [6, 0.8818], + [7, 0.9121], + [8, 0.9424], + [9, 0.9727] + ], + "fallback": 1 + }, + "privileged_user_reaction": { + "cases": [ + [-1, 0.2], + [1, 1] + ], + "fallback": 0.95 + }, + "public_reactions": { + "cases": [ + [0, 0.9988], + [1, 0.9988], + [2, 0.9988], + [3, 0.9988] + ], + "fallback": 1 + } + } +} diff --git a/spec/fixtures/feed-variants/broken/broken-fallback.json b/spec/fixtures/feed-variants/broken/broken-fallback.json new file mode 100644 index 000000000..286648cab --- /dev/null +++ b/spec/fixtures/feed-variants/broken/broken-fallback.json @@ -0,0 +1,26 @@ +{ + "max_days_since_published": 15, + "order_by": "relevancy_score_and_publication_date", + "levers": { + "daily_decay": { + "cases": [ + [0, 1], + [1, 0.99], + [2, 0.985], + [3, 0.98], + [4, 0.975], + [5, 0.97], + [6, 0.965], + [7, 0.96], + [8, 0.955], + [9, 0.95], + [10, 0.945], + [11, 0.94], + [12, 0.935], + [13, 0.93], + [14, 0.925] + ], + "fallback": "OR SELECT * FROM SOMETHING" + } + } +} diff --git a/spec/models/ab_experiment_spec.rb b/spec/models/ab_experiment_spec.rb index 1a0a4eee6..b6b7eac33 100644 --- a/spec/models/ab_experiment_spec.rb +++ b/spec/models/ab_experiment_spec.rb @@ -4,6 +4,19 @@ RSpec.describe AbExperiment do let(:controller) { ApplicationController.new } let(:user) { double } + describe ".get_feed_variant_for" do + before do + allow(controller).to receive(:field_test).with(AbExperiment::CURRENT_FEED_STRATEGY_EXPERIMENT, + participant: user).and_return("special") + end + + it "returns an inquirable string" do + result = described_class.get_feed_variant_for(user: user, controller: controller) + expect(result).to eq("special") + expect(result).to be_special + end + end + describe ".get" do before do allow(controller).to receive(:field_test).with(AbExperiment::CURRENT_FEED_STRATEGY_EXPERIMENT, diff --git a/spec/models/articles/feeds/lever_catalog_builder_spec.rb b/spec/models/articles/feeds/lever_catalog_builder_spec.rb index b1d0f6d53..8e7ff9d83 100644 --- a/spec/models/articles/feeds/lever_catalog_builder_spec.rb +++ b/spec/models/articles/feeds/lever_catalog_builder_spec.rb @@ -3,18 +3,20 @@ require "rails_helper" RSpec.describe Articles::Feeds::LeverCatalogBuilder do let(:catalog) do described_class.new do - order_by_lever(Articles::Feeds::OrderByLever::DEFAULT_KEY, + order_by_lever(:order_by_this, label: "Hello world", order_by_fragment: "relevancy_score DESC") relevancy_lever(:my_key, user_required: true, label: "A label", + range: "[-10..0]", select_fragment: "articles.count") relevancy_lever(:my_other_key, user_required: false, label: "Another label", + range: "[-10..0]", select_fragment: "articles.reactions_count") end end @@ -31,11 +33,13 @@ RSpec.describe Articles::Feeds::LeverCatalogBuilder do relevancy_lever(:my_key, user_required: true, label: "A label", + range: "[-10..0]", select_fragment: "articles.count") relevancy_lever(:my_key, user_required: false, label: "Another label", + range: "[-10..0]", select_fragment: "articles.reactions_count") end end.to raise_error(described_class::DuplicateLeverError) @@ -57,17 +61,17 @@ RSpec.describe Articles::Feeds::LeverCatalogBuilder do end describe "#fetch_lever" do - subject { catalog.fetch_lever(lever) } + subject { catalog.fetch_lever(key) } context "when lever exists" do - let(:lever) { "my_key" } + let(:key) { "my_key" } it { is_expected.to be_a(Articles::Feeds::RelevancyLever) } it { is_expected.to be_frozen } end context "when lever does not exist" do - let(:lever) { "404" } + let(:key) { "404" } it { within_block_is_expected.to raise_error(KeyError) } end @@ -77,15 +81,7 @@ RSpec.describe Articles::Feeds::LeverCatalogBuilder do subject { catalog.fetch_order_by(key) } context "when lever exists" do - let(:key) { Articles::Feeds::OrderByLever::DEFAULT_KEY } - - it { is_expected.to be_a(Articles::Feeds::OrderByLever) } - it { is_expected.to be_frozen } - end - - context "when given a nil key" do - # Note this assumes that we've configured the feed correctly - let(:key) { nil } + let(:key) { "order_by_this" } it { is_expected.to be_a(Articles::Feeds::OrderByLever) } it { is_expected.to be_frozen } diff --git a/spec/models/articles/feeds/order_by_lever_spec.rb b/spec/models/articles/feeds/order_by_lever_spec.rb index b35962814..8cae817b5 100644 --- a/spec/models/articles/feeds/order_by_lever_spec.rb +++ b/spec/models/articles/feeds/order_by_lever_spec.rb @@ -12,4 +12,5 @@ RSpec.describe Articles::Feeds::OrderByLever do it { is_expected.to respond_to :key } it { is_expected.to respond_to :label } it { is_expected.to respond_to :order_by_fragment } + it { is_expected.to respond_to :to_sql } end diff --git a/spec/models/articles/feeds/relevancy_lever_spec.rb b/spec/models/articles/feeds/relevancy_lever_spec.rb index f625c667a..2fdad7e2c 100644 --- a/spec/models/articles/feeds/relevancy_lever_spec.rb +++ b/spec/models/articles/feeds/relevancy_lever_spec.rb @@ -1,9 +1,12 @@ require "rails_helper" RSpec.describe Articles::Feeds::RelevancyLever do - subject do + subject { lever } + + let(:lever) do described_class.new( key: :my_key, + range: "[0..10)", label: "My label", select_fragment: "articles.reaction_count", user_required: true, @@ -13,8 +16,33 @@ RSpec.describe Articles::Feeds::RelevancyLever do it { is_expected.to respond_to :key } it { is_expected.to respond_to :label } it { is_expected.to respond_to :select_fragment } - it { is_expected.to respond_to :joins_fragment } + it { is_expected.to respond_to :joins_fragments } it { is_expected.to respond_to :group_by_fragment } it { is_expected.to respond_to :user_required } it { is_expected.to respond_to :user_required? } + + describe "#configure_with" do + subject { lever.configure_with(fallback: fallback, cases: cases) } + + let(:cases) { [[0, 0]] } + let(:fallback) { 1 } + + context "when fallback is not a number" do + let(:fallback) { "nope" } + + it { within_block_is_expected.to raise_error described_class::InvalidFallbackError } + end + + context "when cases is not an array" do + let(:cases) { "nope" } + + it { within_block_is_expected.to raise_error described_class::InvalidCasesError } + end + + context "when cases includes a non-number" do + let(:cases) { [[0, 1], [1, "a"]] } + + it { within_block_is_expected.to raise_error described_class::InvalidCasesError } + end + end end diff --git a/spec/models/articles/feeds/variant_assembler_spec.rb b/spec/models/articles/feeds/variant_assembler_spec.rb index 4ea393d1b..38cff0569 100644 --- a/spec/models/articles/feeds/variant_assembler_spec.rb +++ b/spec/models/articles/feeds/variant_assembler_spec.rb @@ -2,14 +2,20 @@ require "rails_helper" RSpec.describe Articles::Feeds::VariantAssembler do describe ".call" do - Rails.root.glob("config/feed-variants/*.json").each do |pathname| + Rails.root.glob("#{described_class::DIRECTORY}/*.#{described_class::EXTENSION}").each do |pathname| variant = pathname.basename(".json").to_s.to_sym context "for #{variant.inspect}" do # NOTE: We're providing the variants so as to not pollute the cache for other tests. subject { described_class.call(variant: variant, variants: {}) } - it { is_expected.to be_a(Articles::Feeds::VariantQueryConfig) } + it { is_expected.to be_a(Articles::Feeds::VariantQuery::Config) } end end + + context "with missing variant" do + subject { described_class.call(variant: :obviously_missing, variants: {}) } + + it { within_block_is_expected.to raise_error(Errno::ENOENT) } + end end end diff --git a/spec/requests/stories/feeds_spec.rb b/spec/requests/stories/feeds_spec.rb index 00097c80f..d6b2b56dd 100644 --- a/spec/requests/stories/feeds_spec.rb +++ b/spec/requests/stories/feeds_spec.rb @@ -46,20 +46,26 @@ RSpec.describe "Stories::Feeds", type: :request do ) end - it "returns feed when feed_strategy is optimized" do - allow(Settings::UserExperience).to receive(:feed_strategy).and_return("optimized") + %i[enable disable].each do |toggle| + context "when we #{toggle} :feed_uses_variant_query_feature" do + before { FeatureFlag.public_send(toggle, :feed_uses_variant_query_feature) } - get stories_feed_path + it "returns feed when feed_strategy is not basic" do + allow(Settings::UserExperience).to receive(:feed_strategy).and_return("optimized") - expect(response_article).to include( - "id" => article.id, - "title" => title, - "user_id" => user.id, - "user" => hash_including("name" => user.name), - "organization_id" => organization.id, - "organization" => hash_including("name" => organization.name), - "tag_list" => article.decorate.cached_tag_list_array, - ) + get stories_feed_path + + expect(response_article).to include( + "id" => article.id, + "title" => title, + "user_id" => user.id, + "user" => hash_including("name" => user.name), + "organization_id" => organization.id, + "organization" => hash_including("name" => organization.name), + "tag_list" => article.decorate.cached_tag_list_array, + ) + end + end end context "when rendering an article that is pinned" do diff --git a/spec/services/articles/feeds/variant_query_spec.rb b/spec/services/articles/feeds/variant_query_spec.rb new file mode 100644 index 000000000..8e63639f3 --- /dev/null +++ b/spec/services/articles/feeds/variant_query_spec.rb @@ -0,0 +1,60 @@ +require "rails_helper" + +RSpec.describe Articles::Feeds::VariantQuery, type: :service do + # We're exercising each named feed variant to ensure that the queries are valid SQL. + describe ".build_for" do + Rails.root.glob(File.join(Articles::Feeds::VariantAssembler::DIRECTORY, "/*.json")).each do |pathname| + variant = pathname.basename(".json").to_s.to_sym + subject(:query_call) { variant_query.call } + + let(:variant_query) { described_class.build_for(variant: variant, user: user) } + + context "for #{variant.inspect} and user is nil" do + let(:user) { nil } + + it "is a valid ActiveRecord::Relation" do + article = create(:article) + expect(query_call).to be_a(ActiveRecord::Relation) + expect(query_call.to_a).to match_array(article) + end + end + + context "for #{variant.inspect} and a non-nil user" do + let(:user) { create(:user) } + + it "is a valid ActiveRecord::Relation" do + article = create(:article) + expect(query_call).to be_a(ActiveRecord::Relation) + expect(query_call.to_a).to match_array(article) + end + end + end + + Rails.root.glob("spec/fixtures/feed-variants/broken/*.json").each do |pathname| + variant = pathname.basename(".json").to_s.to_sym + + let(:variant_config) do + # Assembling the config and not polluting the variant cache with broken levers. + Articles::Feeds::VariantAssembler.call(variant: variant, variants: {}, + dir: "spec/fixtures/feed-variants/broken") + end + + # We already assembled the variant's config, let's short circuit that for the query + let(:stubbed_assembler) { ->(*) { variant_config } } + + context "for broken variant #{variant.inspect} and a non-nil user" do + let(:variant_query) { described_class.build_for(variant: variant, user: user, assembler: stubbed_assembler) } + let(:user) { create(:user) } + + it { within_block_is_expected.to raise_error(Articles::Feeds::RelevancyLever::ConfigurationError) } + end + + context "for broken variant #{variant.inspect} and a nil user" do + let(:variant_query) { described_class.build_for(variant: variant, user: user, assembler: stubbed_assembler) } + let(:user) { nil } + + it { within_block_is_expected.to raise_error(Articles::Feeds::RelevancyLever::ConfigurationError) } + end + end + end +end