Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce enable/disable experiment cohorting #615

Merged
merged 2 commits into from
May 27, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions lib/split/dashboard.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,17 @@ class Dashboard < Sinatra::Base
redirect url('/')
end

post '/update_cohorting' do
@experiment = Split::ExperimentCatalog.find(params[:experiment])
case params[:cohorting_action].downcase
when "enable"
@experiment.enable_cohorting
when "disable"
@experiment.disable_cohorting
end
redirect url('/')
end

delete '/experiment' do
@experiment = Split::ExperimentCatalog.find(params[:experiment])
@experiment.delete
Expand Down
10 changes: 10 additions & 0 deletions lib/split/dashboard/public/dashboard.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,13 @@ function confirmReopen() {
var agree = confirm("This will reopen the experiment. Are you sure?");
return agree ? true : false;
}

function confirmEnableCohorting(){
var agree = confirm("This will enable the cohorting of the experiment. Are you sure?");
return agree ? true : false;
}

function confirmDisableCohorting(){
var agree = confirm("This will disable the cohorting of the experiment. Note: Existing participants will continue to receive their alternative and may continue to convert. Are you sure?");
return agree ? true : false;
}
5 changes: 5 additions & 0 deletions lib/split/dashboard/public/style.css
Original file line number Diff line number Diff line change
Expand Up @@ -326,3 +326,8 @@ a.button.green:focus, button.green:focus, input[type="submit"].green:focus {
display: inline-block;
padding: 5px;
}

.divider {
display: inline-block;
margin-left: 10px;
}
13 changes: 13 additions & 0 deletions lib/split/dashboard/views/_controls.erb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,20 @@
<form action="<%= url "/reopen?experiment=#{experiment.name}" %>" method='post' onclick="return confirmReopen()">
<input type="submit" value="Reopen Experiment">
</form>
<% else %>
<% if experiment.cohorting_disabled? %>
<form action="<%= url "/update_cohorting?experiment=#{experiment.name}" %>" method='post' onclick="return confirmEnableCohorting()">
<input type="hidden" name="cohorting_action" value="enable">
<input type="submit" value="Enable Cohorting" class="green">
</form>
<% else %>
<form action="<%= url "/update_cohorting?experiment=#{experiment.name}" %>" method='post' onclick="return confirmDisableCohorting()">
<input type="hidden" name="cohorting_action" value="disable">
<input type="submit" value="Disable Cohorting" class="red">
</form>
<% end %>
<% end %>
<span class="divider">|</span>
<% if experiment.start_time %>
<form action="<%= url "/reset?experiment=#{experiment.name}" %>" method='post' onclick="return confirmReset()">
<input type="submit" value="Reset Data">
Expand Down
23 changes: 23 additions & 0 deletions lib/split/experiment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ def delete
end
reset_winner
redis.srem(:experiments, name)
remove_experiment_cohorting
remove_experiment_configuration
Split.configuration.on_experiment_delete.call(self)
increment_version
Expand Down Expand Up @@ -387,6 +388,23 @@ def jstring(goal = nil)
js_id.gsub('/', '--')
end

def cohorting_disabled?
@cohorting_disabled ||= begin
value = redis.hget(experiment_config_key, :cohorting)
value.nil? ? false : value.downcase == "true"
end
end

def disable_cohorting
@cohorting_disabled = true
redis.hset(experiment_config_key, :cohorting, true)
end

def enable_cohorting
@cohorting_disabled = false
redis.hset(experiment_config_key, :cohorting, false)
end

protected

def experiment_config_key
Expand Down Expand Up @@ -468,5 +486,10 @@ def experiment_configuration_has_changed?
def goals_collection
Split::GoalsCollection.new(@name, @goals)
end

def remove_experiment_cohorting
@cohorting_disabled = false
redis.hdel(experiment_config_key, :cohorting)
end
end
end
19 changes: 13 additions & 6 deletions lib/split/trial.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ def choose!(context = nil)
# Only run the process once
return alternative if @alternative_choosen

new_participant = @user[@experiment.key].nil?
if override_is_alternative?
self.alternative = @options[:override]
if should_store_alternative? && !@user[@experiment.key]
Expand All @@ -70,19 +71,25 @@ def choose!(context = nil)
else
self.alternative = @user[@experiment.key]
if alternative.nil?
self.alternative = @experiment.next_alternative
if @experiment.cohorting_disabled?
andrehjr marked this conversation as resolved.
Show resolved Hide resolved
self.alternative = @experiment.control
else
self.alternative = @experiment.next_alternative

# Increment the number of participants since we are actually choosing a new alternative
self.alternative.increment_participation
# Increment the number of participants since we are actually choosing a new alternative
self.alternative.increment_participation

run_callback context, Split.configuration.on_trial_choose
run_callback context, Split.configuration.on_trial_choose
end
end
end
end

@user[@experiment.key] = alternative.name if [email protected]_winner? && should_store_alternative?
new_participant_and_cohorting_disabled = new_participant && @experiment.cohorting_disabled?

@user[@experiment.key] = alternative.name unless @experiment.has_winner? || !should_store_alternative? || new_participant_and_cohorting_disabled
@alternative_choosen = true
run_callback context, Split.configuration.on_trial unless @options[:disabled] || Split.configuration.disabled?
run_callback context, Split.configuration.on_trial unless @options[:disabled] || Split.configuration.disabled? || new_participant_and_cohorting_disabled
alternative
end

Expand Down
22 changes: 22 additions & 0 deletions spec/dashboard_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,28 @@ def link(color)
end
end

describe "update cohorting" do
it "calls enable of cohorting when action is enable" do
post "/update_cohorting?experiment=#{experiment.name}", { "cohorting_action": "enable" }

expect(experiment.cohorting_disabled?).to eq false
end

it "calls disable of cohorting when action is disable" do
post "/update_cohorting?experiment=#{experiment.name}", { "cohorting_action": "disable" }

expect(experiment.cohorting_disabled?).to eq true
end

it "calls neither enable or disable cohorting when passed invalid action" do
previous_value = experiment.cohorting_disabled?

post "/update_cohorting?experiment=#{experiment.name}", { "cohorting_action": "other" }

expect(experiment.cohorting_disabled?).to eq previous_value
end
end

it "should reset an experiment" do
red_link.participant_count = 5
blue_link.participant_count = 7
Expand Down
36 changes: 35 additions & 1 deletion spec/experiment_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -223,8 +223,14 @@ def alternative(color)
experiment.delete
expect(experiment.start_time).to be_nil
end
end

it "should default cohorting back to false" do
experiment.disable_cohorting
expect(experiment.cohorting_disabled?).to eq(true)
experiment.delete
expect(experiment.cohorting_disabled?).to eq(false)
end
end

describe 'winner' do
it "should have no winner initially" do
Expand Down Expand Up @@ -392,6 +398,34 @@ def alternative(color)
end
end

describe "#cohorting_disabled?" do
it "returns false when nothing has been configured" do
expect(experiment.cohorting_disabled?).to eq false
end

it "returns true when enable_cohorting is performed" do
experiment.enable_cohorting
expect(experiment.cohorting_disabled?).to eq false
end

it "returns false when nothing has been configured" do
experiment.disable_cohorting
expect(experiment.cohorting_disabled?).to eq true
end
end

describe "#disable_cohorting" do
it "saves a new key in redis" do
expect(experiment.disable_cohorting).to eq true
end
end

describe "#enable_cohorting" do
it "saves a new key in redis" do
expect(experiment.enable_cohorting).to eq true
end
end

describe 'changing an existing experiment' do
def same_but_different_alternative
Split::ExperimentCatalog.find_or_create('link_color', 'blue', 'yellow', 'orange')
Expand Down
27 changes: 27 additions & 0 deletions spec/trial_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,33 @@ def expect_alternative(trial, alternative_name)
expect(trial.alternative.name).to_not be_empty
Split.configuration.on_trial_choose = nil
end

it "assigns user to an alternative" do
trial.choose! context

expect(alternatives).to include(user[experiment.name])
end

context "when cohorting is disabled" do
before(:each) { allow(experiment).to receive(:cohorting_disabled?).and_return(true) }

it "picks the control and does not run on_trial callbacks" do
Split.configuration.on_trial = :on_trial_callback

expect(experiment).to_not receive(:next_alternative)
expect(context).not_to receive(:on_trial_callback)
expect_alternative(trial, 'basket')

Split.configuration.enabled = true
Split.configuration.on_trial = nil
end

it "user is not assigned an alternative" do
trial.choose! context

expect(user[experiment]).to eq(nil)
end
end
end
end

Expand Down