From 3fb085b3d92b4cea1d3f0ad82b109b8aff3747c8 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Fri, 3 Jun 2022 11:27:09 -0400 Subject: [PATCH] Adding more context to `/admin/abtests` (#17821) * Adding more context to `/admin/abtests` Prior to this commit, the `/admin/abtests` was fully rendered by the field_test gem (see https://github.com/ankane/field_test/blob/master/app/views/field_test/experiments/index.html.erb and https://github.com/ankane/field_test/blob/master/app/views/field_test/experiments/show.html.erb). With this commit, we're pre-pending our own partial (e.g. `app/views/field_test/experiments/_experiments.html.erb`) in the view paths; which means instead of rendering https://github.com/ankane/field_test/blob/master/app/views/field_test/experiments/_experiments.html.erb we render our newly created `app/views/field_test/experiments/_experiments.html.erb`) Why the ugly antics in the view? As I'm not fully certain if this will "meet" the full needs of those monitoring the experiments. I'm also constructing this to most closely match the spreadsheet that has been assembled for tracking this information. Why introduce another caching layer for the variant assembly? The original caching layer (e.g. `Articles::Feeds::VariantAssembler.pre_assembled_variants`) is in place to provide the quickest access to the assembled variants. But due to the nature of the implemented view, we are needing a programmatic representation of all variants. And the added Rails cache is there to minimize disk reads. Closes forem/forem#17820 * Update app/models/articles/feeds/variant_assembler.rb Co-authored-by: Ridhwana * Update app/models/articles/feeds/variant_assembler.rb Co-authored-by: Ridhwana Co-authored-by: Ridhwana --- .../articles/feeds/variant_assembler.rb | 43 +++++-- app/services/articles/feeds/variant_query.rb | 1 + .../experiments/_experiments.html.erb | 118 ++++++++++++++++++ .../articles/feeds/variant_assembler_spec.rb | 19 ++- 4 files changed, 173 insertions(+), 8 deletions(-) create mode 100644 app/views/field_test/experiments/_experiments.html.erb diff --git a/app/models/articles/feeds/variant_assembler.rb b/app/models/articles/feeds/variant_assembler.rb index 2f52f2c46..3004561b8 100644 --- a/app/models/articles/feeds/variant_assembler.rb +++ b/app/models/articles/feeds/variant_assembler.rb @@ -11,6 +11,12 @@ module Articles # The default extension for feed variants EXTENSION = "json".freeze + # We have a "historical variant" that we renamed, hence we continue to maintain that name in + # our data through the following map. + VARIANT_NAME_MAP = { + "20220422-jennie-variant": :"20220422-variant" + }.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 @@ -23,20 +29,42 @@ module Articles # 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) + # @see .experiment_config_hash_for + def self.call(variant:, catalog: Articles::Feeds.lever_catalog, variants: pre_assembled_variants, **kwargs) variant = variant.to_sym variants[variant] ||= begin - content = Rails.root.join(dir, "#{variant}.#{EXTENSION}").read - config = JSON.parse(content) + config = user_config_hash_for(variant: variant, **kwargs) build_with(catalog: catalog, config: config, variant: variant) end end - # @return [Hash] - def self.variants_cache - @variants_cache ||= {} + # @param variant [#to_sym,String,Symbol] the name of the variant we're assembling + # @param dir [String] the relative directory that contains the variants. + # + # @return [Hash] + # + # @note Uses Rails.cache to minimize reads from file system. The reason for the Rails.cache + # and not leveraging the .pre_assembled_variants is that this method + # (e.g. .experiment_config_hash_for) handles all possible variant configurations (in + # contrast to the active variants). + # + # @see app/views/field_test/experiments/_experiments.html.erb + def self.user_config_hash_for(variant:, dir: DIRECTORY) + Rails.cache.fetch("feed-variant-#{variant}-#{ForemInstance.latest_commit_id}", expires_in: 24.hours) do + variant = VARIANT_NAME_MAP.fetch(variant.to_sym, variant) + content = Rails.root.join(dir, "#{variant}.#{EXTENSION}").read + JSON.parse(content) + end end - private_class_method :variants_cache + + # A memoized (e.g. cached) module instance variable that provides the quickest access for + # already assembled and active variant configurations. + # + # @return [Hash] + def self.pre_assembled_variants + @pre_assembled_variants ||= {} + end + private_class_method :pre_assembled_variants # @param catalog [Articles::Feeds::LeverCatalogBuilder] # @param variant [Symbol] @@ -54,6 +82,7 @@ module Articles VariantQuery::Config.new( variant: variant, levers: relevancy_levers, + description: config.fetch("description", ""), order_by: catalog.fetch_order_by(config.fetch("order_by")), max_days_since_published: config.fetch("max_days_since_published"), ) diff --git a/app/services/articles/feeds/variant_query.rb b/app/services/articles/feeds/variant_query.rb index d2af5b2e3..ba877b54f 100644 --- a/app/services/articles/feeds/variant_query.rb +++ b/app/services/articles/feeds/variant_query.rb @@ -30,6 +30,7 @@ module Articles Config = Struct.new( :variant, + :description, :levers, # Array :order_by, # Articles::Feeds::OrderByLever :max_days_since_published, diff --git a/app/views/field_test/experiments/_experiments.html.erb b/app/views/field_test/experiments/_experiments.html.erb new file mode 100644 index 000000000..6a1806491 --- /dev/null +++ b/app/views/field_test/experiments/_experiments.html.erb @@ -0,0 +1,118 @@ + +<% experiments.reverse_each do |experiment| %> +

<%= experiment.name %><% unless experiment.active? %> Completed<% end %>

+ > + Experiment Details + + <% if experiment.description %> +

<%= experiment.description %>

+ <% end %> + + > + Goals for <%= experiment.name %> + <% experiment.goals.each do |goal| %> + <% results = experiment.results(goal: goal) %> + + <% if experiment.multiple_goals? %> +

<%= goal.titleize %>

+ <% end %> + + + + + + + + + + + + + <% results.each do |variant, result| %> + + + + + + + + <% end %> + +
VariantParticipantsConversionsConversion RateProb Winning
+ <%= variant %> + <% if variant == experiment.winner %> + ✓ + <% end %> + <%= result[:participated] %><%= result[:converted] %> + <% if result[:conversion_rate] %> + <%= (100.0 * result[:conversion_rate]).round(FieldTest.precision) %>% + <% else %> + - + <% end %> + + <% if result[:prob_winning] %> + <% if result[:prob_winning] < 0.01 %> + < 1% + <% else %> + <%= (100.0 * result[:prob_winning]).round(FieldTest.precision) %>% + <% end %> + <% end %> +
+ <% end %> + + + <% if experiment.id.start_with?("feed_strategy") %> + <% experiment.variants.each do |variant| %> +
+ Config for <%= variant %> <% if variant == experiment.winner %>✓<% end %> + <% config = Articles::Feeds::VariantAssembler.user_config_hash_for(variant: variant) %> +
+
Description
+
<%= config["description"] || "n/a" %>
+
Weight
+
<%= experiment.weights[experiment.variants.index(variant)] %>
+
Order Lever
+
<%= config["order_by"] %>
+ <% config["levers"].each do |lever_key, lever| %> + <% next unless lever.key?("query_parameters") %> +
Parameters for <%= lever_key %> relevancy lever
+ <% lever["query_parameters"].each do |key, value| %> +
<%= key %>: <%= value %>
+ <% end %> + <% end %> +
+ + + <% lever_range = config["levers"].map { |_, lever| lever["cases"].map(&:first) }.flatten.uniq.sort %> + + + + + + <% lever_range.each do |i| %> + + <% end %> + + + + <% config["levers"].each do |lever_key, lever| %> + <% cases = lever["cases"].each_with_object({}) { |(key, value), mem| mem[key] = value } %> + + + + <% end %> + + <% end %> + +
Relevency Lever(s): Fallback and Range Factors
LeverFallback<%= i %>
<%= lever_key %><%= lever["fallback"] %> + <% lever_range.each do |i| %> + <%= cases.fetch(i, " ".html_safe) %>
+
+ <% end %> + <% end %> + +

<%= link_to "Data Details", experiment_path(experiment.id) %> (individual user conversions)

+ +<% end %> diff --git a/spec/models/articles/feeds/variant_assembler_spec.rb b/spec/models/articles/feeds/variant_assembler_spec.rb index 38cff0569..497f673de 100644 --- a/spec/models/articles/feeds/variant_assembler_spec.rb +++ b/spec/models/articles/feeds/variant_assembler_spec.rb @@ -1,10 +1,27 @@ require "rails_helper" RSpec.describe Articles::Feeds::VariantAssembler do + describe ".user_config_hash_for" do + Rails.root.glob("#{described_class::DIRECTORY}/*.#{described_class::EXTENSION}").each do |pathname| + variant = pathname.basename(".json").to_s + context "when #{variant.inspect}" do + subject { described_class.user_config_hash_for(variant: variant) } + + it { is_expected.to be_a(Hash) } + end + end + + context "when \"20220422-jennie-variant\"" do + subject { described_class.user_config_hash_for(variant: "20220422-jennie-variant") } + + it { is_expected.to be_a(Hash) } + end + end + describe ".call" do 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 + context "when #{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: {}) }