diff --git a/app/models/ab_experiment.rb b/app/models/ab_experiment.rb index 2072b02d2..f9bede301 100644 --- a/app/models/ab_experiment.rb +++ b/app/models/ab_experiment.rb @@ -98,6 +98,20 @@ class AbExperiment < SimpleDelegator .public_send(method_name, user: user, default_value: default_value, experiment: experiment, config: config) end + # Responsible for checking if a given :user has "accomplished" the state :goal for any of the + # active :experiments. We only consider events that occur on or after each experiment's given + # start_date. + # + # @param user [User] + # @param goal [String] + # @param experiments [Hash] + # + # @see AbExperiment::GoalConversionChecker + # @see FieldTest.config + def self.register_conversions_for(user:, goal:, experiments: FieldTest.config["experiments"]) + GoalConversionHandler.call(user: user, goal: goal, experiments: experiments) + end + # @api private # @param controller [ApplicationController] the current controller # that's handling the current request. diff --git a/app/models/ab_experiment/goal_conversion_handler.rb b/app/models/ab_experiment/goal_conversion_handler.rb new file mode 100644 index 000000000..038335ccb --- /dev/null +++ b/app/models/ab_experiment/goal_conversion_handler.rb @@ -0,0 +1,93 @@ +class AbExperiment + # Responsible for checking if a given :user has "accomplished" the state :goal for any of the + # active :experiments. We scope our tests to events that happened on or after the experiment's + # start date. + # + # @note It is required that each experiment have a start date (in CCYY-MM-DD format). + class GoalConversionHandler + include FieldTest::Helpers + + USER_CREATES_PAGEVIEW_GOAL = "user_creates_pageview".freeze + USER_CREATES_COMMENT_GOAL = "user_creates_comment".freeze + + def self.call(...) + new(...).call + end + + def initialize(user:, goal:, experiments:) + @user = user + @goal = goal + @experiments = experiments + end + + attr_reader :experiments, :user, :goal + + def call + # It's okay that there are no experiments. + return if experiments.nil? + + experiments.each do |key, data| + experiment_start_date = data.fetch("start_date").beginning_of_day + experiment = key.to_sym + convert(experiment: experiment, experiment_start_date: experiment_start_date) + end + end + + private + + def convert(experiment:, experiment_start_date:) + case goal + # We have special conditional goals for some where we look for past events for cummulative wins + # Otherwise we convert the goal as given. + when USER_CREATES_PAGEVIEW_GOAL + pageview_goal(experiment, + [7.days.ago, experiment_start_date].max, + "DATE(created_at)", + 4, + "user_views_pages_on_at_least_four_different_days_within_a_week") + pageview_goal(experiment, + [24.hours.ago, experiment_start_date].max, + "DATE_PART('hour', created_at)", + 4, + "user_views_pages_on_at_least_four_different_hours_within_a_day") + pageview_goal(experiment, + [14.days.ago, experiment_start_date].max, + "DATE(created_at)", + 9, + "user_views_pages_on_at_least_nine_different_days_within_two_weeks") + pageview_goal(experiment, + [5.days.ago, experiment_start_date].max, + "DATE_PART('hour', created_at)", + 12, + "user_views_pages_on_at_least_twelve_different_hours_within_five_days") + when USER_CREATES_COMMENT_GOAL # comments goal. Only page views and comments are currently active. + field_test_converted(experiment, participant: user, goal: goal) # base single comment goal. + comment_goal(experiment, + [7.days.ago, experiment_start_date].max, + "DATE(created_at)", + 4, + "user_creates_comment_on_at_least_four_different_days_within_a_week") + else + field_test_converted(experiment, participant: user, goal: goal) # base single comment goal. + end + end + + def pageview_goal(experiment, time_start, group_value, min_count, goal) + page_view_counts = user.page_views.where("created_at > ?", time_start) + .group(group_value).count.values + page_view_counts.delete(0) + return unless page_view_counts.size >= min_count + + field_test_converted(experiment, participant: user, goal: goal) + end + + def comment_goal(experiment, time_start, group_value, min_count, goal) + comment_counts = user.comments.where("created_at > ?", time_start) + .group(group_value).count.values + comment_counts.delete(0) + return unless comment_counts.size >= min_count + + field_test_converted(experiment, participant: user, goal: goal) + end + end +end diff --git a/app/models/comment.rb b/app/models/comment.rb index f72dcfbb5..73abfdb34 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -346,7 +346,7 @@ class Comment < ApplicationRecord return if FieldTest.config["experiments"].nil? Users::RecordFieldTestEventWorker - .perform_async(user_id, "user_creates_comment") + .perform_async(user_id, AbExperiment::GoalConversionHandler::USER_CREATES_COMMENT_GOAL) end def notify_slack_channel_about_warned_users diff --git a/app/models/page_view.rb b/app/models/page_view.rb index 1fe745faa..315fe1427 100644 --- a/app/models/page_view.rb +++ b/app/models/page_view.rb @@ -30,12 +30,13 @@ class PageView < ApplicationRecord article.decorate.cached_tag_list_array end + # @see AbExperiment::GoalConversionHandler def record_field_test_event return if FieldTest.config["experiments"].nil? return unless user_id Users::RecordFieldTestEventWorker - .perform_async(user_id, "user_creates_pageview") + .perform_async(user_id, AbExperiment::GoalConversionHandler::USER_CREATES_PAGEVIEW_GOAL) end end diff --git a/app/workers/users/record_field_test_event_worker.rb b/app/workers/users/record_field_test_event_worker.rb index 9eb8a5b95..57144f665 100644 --- a/app/workers/users/record_field_test_event_worker.rb +++ b/app/workers/users/record_field_test_event_worker.rb @@ -1,57 +1,14 @@ module Users class RecordFieldTestEventWorker include Sidekiq::Job - include FieldTest::Helpers sidekiq_options queue: :low_priority, retry: 10 def perform(user_id, goal) - @user = User.find_by(id: user_id) - return unless @user - return unless FieldTest.config["experiments"] + user = User.find_by(id: user_id) + return unless user - FieldTest.config["experiments"].each_key do |key| - @experiment = key.to_sym - case goal - # We have special conditional goals for some where we look for past events for commulative wins - # Otherwise we convert the goal as given. - when "user_creates_pageview" - pageview_goal(7.days.ago, "DATE(created_at)", 4, - "user_views_pages_on_at_least_four_different_days_within_a_week") - pageview_goal(24.hours.ago, "DATE_PART('hour', created_at)", 4, - "user_views_pages_on_at_least_four_different_hours_within_a_day") - pageview_goal(14.days.ago, "DATE(created_at)", 9, - "user_views_pages_on_at_least_nine_different_days_within_two_weeks") - pageview_goal(5.days.ago, "DATE_PART('hour', created_at)", 12, - "user_views_pages_on_at_least_twelve_different_hours_within_five_days") - when "user_creates_comment" # comments goal. Only page views and comments are currently active. - field_test_converted(@experiment, participant: @user, goal: goal) # base single comment goal. - comment_goal(7.days.ago, "DATE(created_at)", 4, - "user_creates_comment_on_at_least_four_different_days_within_a_week") - else - field_test_converted(@experiment, participant: @user, goal: goal) # base single comment goal. - end - end - end - - private - - def pageview_goal(time_start, group_value, min_count, goal_name) - page_view_counts = @user.page_views.where("created_at > ?", time_start) - .group(group_value).count.values - page_view_counts.delete(0) - return unless page_view_counts.size >= min_count - - field_test_converted(@experiment, participant: @user, goal: goal_name) - end - - def comment_goal(time_start, group_value, min_count, goal_name) - comment_counts = @user.comments.where("created_at > ?", time_start) - .group(group_value).count.values - comment_counts.delete(0) - return unless comment_counts.size >= min_count - - field_test_converted(@experiment, participant: @user, goal: goal_name) + AbExperiment.register_conversions_for(user: user, goal: goal) end end end diff --git a/config/field_test.yml b/config/field_test.yml index 3c9d38aa6..4a060905d 100644 --- a/config/field_test.yml +++ b/config/field_test.yml @@ -1,11 +1,10 @@ +# See AbExperiment namespace for details of goals and variants experiments: - # No field test running. - # To run a test, change the five variants here. You can create more than - # five, but five is our typical standard. - # Do not change the goals unless you intend to also change the corresponding code in - # app/workers/users/record_field_test_event_worker.rb - + # NOTE: Our feed strategy testing experiment must begin with "feed_strategy" feed_strategy_starting_20220422: + # NOTE: Required as we want only want to consider for conversion events that + # occurred on or after the given start_date. + start_date: 2022-04-22 variants: - 20220415-incumbent - 20220422-jennie-variant diff --git a/spec/models/ab_experiment/goal_conversion_handler_spec.rb b/spec/models/ab_experiment/goal_conversion_handler_spec.rb new file mode 100644 index 000000000..7543052d3 --- /dev/null +++ b/spec/models/ab_experiment/goal_conversion_handler_spec.rb @@ -0,0 +1,151 @@ +require "rails_helper" +RSpec.describe AbExperiment::GoalConversionHandler do + include FieldTest::Helpers + + describe ".call" do + subject(:handler) { described_class.new(user: user, goal: goal, experiments: experiments) } + + let(:user) { create(:user) } + let(:goal) { "non_sense" } + let(:experiments) { FieldTest.config["experiments"] } + + context "with no experiments" do + let(:experiments) { nil } + + it "gracefully handles a case where there are no tests" do + handler.call + expect(FieldTest::Event.all.size).to be(0) + end + end + + context "with experiment that started to soon for some results" do + let(:goal) { described_class::USER_CREATES_COMMENT_GOAL } + let(:experiment_name) { AbExperiment::CURRENT_FEED_STRATEGY_EXPERIMENT } + # NOTE: We're choosing a future date as a logic short-cut for comment tests + let(:experiments) { { experiment_name => { "start_date" => 8.days.from_now } } } + + before do + field_test(experiment_name, participant: user) + end + + it "registers some events but not others", :aggregate_failures do + 7.times do |n| + create(:comment, user_id: user.id, created_at: n.days.ago) + end + handler.call + # Only the comment registered; the other one would've registered had the events happened + # after the experiment start date. + expect(FieldTest::Event.where(name: goal).count).to eq(1) + expect(FieldTest::Event + .where(name: "user_creates_comment_on_at_least_four_different_days_within_a_week") + .count).to eq(0) + end + end + + context "with user who is part of field test and user_creates_comment goal" do + let(:goal) { described_class::USER_CREATES_COMMENT_GOAL } + + before do + field_test(AbExperiment::CURRENT_FEED_STRATEGY_EXPERIMENT, participant: user) + end + + it "records a conversion", :aggregate_failures do + handler.call + expect(FieldTest::Event.last.field_test_membership.participant_id).to eq(user.id.to_s) + expect(FieldTest::Event.last.name).to eq(goal) + end + + it "records user_creates_comment_on_at_least_four_different_days_within_a_week field test conversion", :aggregate_failures do + 7.times do |n| + create(:comment, user_id: user.id, created_at: n.days.ago) + end + handler.call + expect(FieldTest::Event.last.field_test_membership.participant_id).to eq(user.id.to_s) + expect(FieldTest::Event.last.name) + .to eq("user_creates_comment_on_at_least_four_different_days_within_a_week") + end + end + + context "with user who is part of field test and user_creates_pageview goal" do + let(:goal) { described_class::USER_CREATES_PAGEVIEW_GOAL } + + before do + field_test(AbExperiment::CURRENT_FEED_STRATEGY_EXPERIMENT, participant: user) + end + + it "records user_views_pages_on_at_least_four_different_days_within_a_week field test conversion", :aggregate_failures do + 7.times do |n| + create(:page_view, user_id: user.id, created_at: n.days.ago) + end + handler.call + expect(FieldTest::Event.last.field_test_membership.participant_id).to eq(user.id.to_s) + expect(FieldTest::Event.pluck(:name)) + .to include("user_views_pages_on_at_least_four_different_days_within_a_week") + end + + it "records user_views_pages_on_at_least_nine_different_days_within_two_weeks field test conversionn", :aggregate_failures do + 10.times do |n| + create(:page_view, user_id: user.id, created_at: n.days.ago) + end + handler.call + expect(FieldTest::Event.last.field_test_membership.participant_id).to eq(user.id.to_s) + expect(FieldTest::Event.pluck(:name)) + .to include("user_views_pages_on_at_least_nine_different_days_within_two_weeks") + end + + it "records user_views_pages_on_at_least_twelve_different_hours_within_five_days field test conversion", :aggregate_failures do + 15.times do |n| + create(:page_view, user_id: user.id, created_at: n.hours.ago) + end + + handler.call + expect(FieldTest::Event.last.field_test_membership.participant_id).to eq(user.id.to_s) + expect(FieldTest::Event.pluck(:name)) + .to include("user_views_pages_on_at_least_twelve_different_hours_within_five_days") + end + + it "does not record field test conversion if not qualifying" do + 2.times do |n| + create(:page_view, user_id: user.id, created_at: n.days.ago) + end + handler.call + expect(FieldTest::Event.all.size).to be(0) + end + + it "records user_views_pages_on_at_least_four_different_hours_within_a_day field test conversionn", :aggregate_failures do + 7.times do |n| + create(:page_view, user_id: user.id, created_at: n.hours.ago) + end + handler.call + expect(FieldTest::Event.last.field_test_membership.participant_id).to eq(user.id.to_s) + expect(FieldTest::Event.pluck(:name)) + .to include("user_views_pages_on_at_least_four_different_hours_within_a_day") + end + + it "does not record user_views_article_four_hours_in_day field test conversion for non-qualifying activity" do + 2.times do |n| + create(:page_view, user_id: user.id, created_at: n.hours.ago) + end + handler.call + expect(FieldTest::Event.all.size).to be(0) + end + end + + context "with user who is not part of field test" do + let(:goal) { described_class::USER_CREATES_COMMENT_GOAL } + + it "does not register a conversion" do + handler.call + expect(FieldTest::Event.all.size).to be(0) + end + + it "records user_views_article_four_days_in_week field test conversion" do + 7.times do |n| + create(:page_view, user_id: user.id, created_at: n.days.ago) + end + handler.call + expect(FieldTest::Event.all.size).to be(0) + end + end + end +end diff --git a/spec/models/ab_experiment_spec.rb b/spec/models/ab_experiment_spec.rb index b6b7eac33..efbb6cef2 100644 --- a/spec/models/ab_experiment_spec.rb +++ b/spec/models/ab_experiment_spec.rb @@ -4,6 +4,15 @@ RSpec.describe AbExperiment do let(:controller) { ApplicationController.new } let(:user) { double } + describe ".register_conversions_for" do + it "forwards delegates to Converter.call" do + allow(described_class::GoalConversionHandler).to receive(:call) + described_class.register_conversions_for(user: user, goal: "goal") + expect(described_class::GoalConversionHandler) + .to have_received(:call).with(user: user, goal: "goal", experiments: FieldTest.config["experiments"]) + end + end + describe ".get_feed_variant_for" do before do allow(controller).to receive(:field_test).with(AbExperiment::CURRENT_FEED_STRATEGY_EXPERIMENT, diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 0d1e0e63c..348e3a144 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -202,15 +202,16 @@ RSpec.configure do |config| # Default to have field a field test available. if AbExperiment::CURRENT_FEED_STRATEGY_EXPERIMENT.blank? config = { "experiments" => - { "wut" => - { "variants" => %w[base var_1], - "weights" => [50, 50], - "goals" => %w[user_creates_comment - user_creates_comment_four_days_in_week - user_views_article_four_days_in_week - user_views_article_four_hours_in_day - user_views_article_nine_days_in_two_week - user_views_article_twelve_hours_in_five_days] } }, + { "wut" => + { "start_date" => 30.days.ago, + "variants" => %w[base var_1], + "weights" => [50, 50], + "goals" => %w[user_creates_comment + user_creates_comment_four_days_in_week + user_views_article_four_days_in_week + user_views_article_four_hours_in_day + user_views_article_nine_days_in_two_week + user_views_article_twelve_hours_in_five_days] } }, "exclude" => { "bots" => true }, "cache" => true, "cookies" => false } diff --git a/spec/workers/users/record_field_test_event_worker_spec.rb b/spec/workers/users/record_field_test_event_worker_spec.rb index a8f806799..4c2b35729 100644 --- a/spec/workers/users/record_field_test_event_worker_spec.rb +++ b/spec/workers/users/record_field_test_event_worker_spec.rb @@ -6,120 +6,28 @@ RSpec.describe Users::RecordFieldTestEventWorker, type: :worker do describe "#perform" do let(:worker) { subject } + let(:goal) { "try_to_takeover_the_world" } - let(:user) { create(:user) } + before do + allow(AbExperiment).to receive(:register_conversions_for).and_call_original + end - context "with no field tests configured" do - it "gracefully handles a case where there are no tests" do - allow(FieldTest).to receive(:config).and_return({ "experiments" => nil }) - worker.perform(user.id, "user_creates_reaction") + context "with a non-existent user" do + let(:user_id) { nil } + + it "gracefully exits" do + worker.perform(user_id, goal) + expect(AbExperiment).not_to have_received(:register_conversions_for) end end - context "with user who is part of field test" do - before do - field_test(AbExperiment::CURRENT_FEED_STRATEGY_EXPERIMENT, participant: user) - end + context "with a user" do + let(:user) { create(:user) } + let(:user_id) { user.id } - it "records user_creates_reaction field test conversion" do - worker.perform(user.id, "user_creates_reaction") - expect(FieldTest::Event.last.field_test_membership.participant_id).to eq(user.id.to_s) - expect(FieldTest::Event.last.name).to eq("user_creates_reaction") - end - - it "records user_creates_comment field test conversion" do - worker.perform(user.id, "user_creates_comment") - expect(FieldTest::Event.last.field_test_membership.participant_id).to eq(user.id.to_s) - expect(FieldTest::Event.last.name).to eq("user_creates_comment") - end - - it "records user_creates_comment_on_at_least_four_different_days_within_a_week field test conversion if qualifies" do - 7.times do |n| - create(:comment, user_id: user.id, created_at: n.days.ago) - end - worker.perform(user.id, "user_creates_comment") - expect(FieldTest::Event.last.field_test_membership.participant_id).to eq(user.id.to_s) - expect(FieldTest::Event.last.name).to eq("user_creates_comment_on_at_least_four_different_days_within_a_week") - end - - it "records user_views_pages_on_at_least_four_different_days_within_a_week field test conversion if qualifies" do - 7.times do |n| - create(:page_view, user_id: user.id, created_at: n.days.ago) - end - worker.perform(user.id, "user_creates_pageview") - expect(FieldTest::Event.last.field_test_membership.participant_id).to eq(user.id.to_s) - expect(FieldTest::Event.pluck(:name)).to include("user_views_pages_on_at_least_four_different_days_within_a_week") - end - - it "records user_views_pages_on_at_least_nine_different_days_within_two_weeks field test conversion if qualifies" do - 10.times do |n| - create(:page_view, user_id: user.id, created_at: n.days.ago) - end - worker.perform(user.id, "user_creates_pageview") - expect(FieldTest::Event.last.field_test_membership.participant_id).to eq(user.id.to_s) - expect(FieldTest::Event.pluck(:name)).to include("user_views_pages_on_at_least_nine_different_days_within_two_weeks") - end - - it "records user_views_pages_on_at_least_twelve_different_hours_within_five_days field test conversion if qualifies" do - 15.times do |n| - create(:page_view, user_id: user.id, created_at: n.hours.ago) - end - - worker.perform(user.id, "user_creates_pageview") - expect(FieldTest::Event.last.field_test_membership.participant_id).to eq(user.id.to_s) - expect(FieldTest::Event.pluck(:name)).to include("user_views_pages_on_at_least_twelve_different_hours_within_five_days") - end - - it "does not record field test conversion if not qualifying" do - 2.times do |n| - create(:page_view, user_id: user.id, created_at: n.days.ago) - end - worker.perform(user.id, "user_creates_pageview") - expect(FieldTest::Event.all.size).to be(0) - end - - it "records user_views_pages_on_at_least_four_different_hours_within_a_day field test conversion if qualifies" do - 7.times do |n| - create(:page_view, user_id: user.id, created_at: n.hours.ago) - end - worker.perform(user.id, "user_creates_pageview") - expect(FieldTest::Event.last.field_test_membership.participant_id).to eq(user.id.to_s) - expect(FieldTest::Event.pluck(:name)).to include("user_views_pages_on_at_least_four_different_hours_within_a_day") - end - - it "does not record user_views_article_four_hours_in_day field test conversion if not qualifying" do - 2.times do |n| - create(:page_view, user_id: user.id, created_at: n.hours.ago) - end - worker.perform(user.id, "user_creates_pageview") - expect(FieldTest::Event.all.size).to be(0) - end - end - - context "with user who is not part of field test" do - it "records user_creates_reaction field test conversion" do - worker.perform(user.id, "user_creates_reaction") - expect(FieldTest::Event.all.size).to be(0) - end - - it "records user_creates_comment field test conversion" do - worker.perform(user.id, "user_creates_comment") - expect(FieldTest::Event.all.size).to be(0) - end - - it "records user_views_article_four_days_in_week field test conversion if qualifies" do - 7.times do |n| - create(:page_view, user_id: user.id, created_at: n.days.ago) - end - expect(FieldTest::Event.all.size).to be(0) - end - end - - context "without a user" do - it "does not raise an error" do - expect do - worker.perform(user.id + 1000, "user_creates_reaction") - end.not_to raise_error + it "forward delegates to AbExperiment" do + worker.perform(user_id, goal) + expect(AbExperiment).to have_received(:register_conversions_for).with(user: user, goal: goal) end end end