Moving Experiments into AbExperiment namespace (#17532)

* Moving Experiments into AbExperiment namespace

This commit entails two major concepts:

1. Extracting logic out of a worker.
2. Cleaning the conditions in which we convert experiments.

In addition, I revisited some spec names to tidy them up for
clarification and structure.

**Extracting Logic...**

For workers, my preference is that they be a simple router to another
object.  A non-worker object, with it's initialize method, can both
better manage instance variables.  Further, workers are somewhat "flat"
in hierarchy,

In this case, having the AbExperiment be knowledgeable both in setting
the experiment and handling conversions makes (to me) organizational
sense.  For example, the AbExperiment is constructed as a wrapper to the
FieldTest gem.  But the RecordFieldTestEventWorker had knowledge of
FieldTest.

With this refactor, it does not have that knowledge.  This also provided
an opportunity to replace magic strings with constants.

**Cleaning the conditions...**

This relates directly to and closes forem/forem#17530.  I was looking at
the experiments after 2 days, and realized that there shouldn't be
results for
`user_views_pages_on_at_least_nine_different_days_within_two_weeks`;
after all the experiment started 2 days ago, how can we have results
that look at 2 weeks.

By adding the "max" function calls, we ensure a clear boundary of
"before the experiment began" versus "while the experiment is running".

Closes forem/forem#17530

* Update spec/models/ab_experiment_spec.rb

Co-authored-by: Mac Siri <krairit.siri@gmail.com>

* Apply suggestions from code review

Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>

Co-authored-by: Mac Siri <krairit.siri@gmail.com>
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>

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<String, Object>]
+  #
+  # @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.

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

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

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

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

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

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

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,

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 }

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