From 99ae0643ee3dba24caa146f5eb17daaed149f625 Mon Sep 17 00:00:00 2001 From: John Mileham Date: Sun, 5 May 2019 10:06:06 -0400 Subject: [PATCH 1/2] Don't merge feature gate assignments when superseding visitors --- app/models/assignment.rb | 2 + app/models/split.rb | 2 + app/models/visitor_supersession.rb | 4 +- spec/models/visitor_supersession_spec.rb | 67 ++++++++++++++++++------ 4 files changed, 58 insertions(+), 17 deletions(-) diff --git a/app/models/assignment.rb b/app/models/assignment.rb index 8e7b4aad..bf767123 100644 --- a/app/models/assignment.rb +++ b/app/models/assignment.rb @@ -51,6 +51,8 @@ class Assignment < ActiveRecord::Base ) end + scope :except_feature_gates, -> { joins(:split).merge(Split.except_feature_gates) } + def variant_detail @variant_detail ||= begin detail = variant_details.select { |d| d.variant == variant }.first diff --git a/app/models/split.rb b/app/models/split.rb index d2efe4e0..77aabfe0 100644 --- a/app/models/split.rb +++ b/app/models/split.rb @@ -77,6 +77,8 @@ def self.arel_excluding_remote_kills_for(app_build) .where(AppRemoteKill.arel_table[:split_id].eq(arel_table[:id])).exists.not end + scope :except_feature_gates, -> { where(feature_gate: false) } + def detail @detail ||= SplitDetail.new(split: self) end diff --git a/app/models/visitor_supersession.rb b/app/models/visitor_supersession.rb index 4853838d..997e250e 100644 --- a/app/models/visitor_supersession.rb +++ b/app/models/visitor_supersession.rb @@ -7,8 +7,8 @@ class VisitorSupersession < ActiveRecord::Base private def merge_assignments! - target_split_ids = superseding_visitor.assignments.map(&:split_id).to_set - superseded_visitor.assignments.order(:id).each do |a| + target_split_ids = superseding_visitor.assignments.pluck(:split_id).to_set + superseded_visitor.assignments.except_feature_gates.order(:id).each do |a| create_or_ignore_duplicate(a) unless target_split_ids.include?(a.split_id) end end diff --git a/spec/models/visitor_supersession_spec.rb b/spec/models/visitor_supersession_spec.rb index f395db66..9746aaec 100644 --- a/spec/models/visitor_supersession_spec.rb +++ b/spec/models/visitor_supersession_spec.rb @@ -8,9 +8,59 @@ let(:banana_split) { FactoryBot.create(:split, name: :banana, registry: { green: 50, squishy: 50 }) } let(:torque_split) { FactoryBot.create(:split, name: :torque, registry: { front: 40, rear: 60 }) } + let(:decided_split) { FactoryBot.create(:split, name: :decided, registry: { bad_thing: 50, good_thing: 50 }) } + let(:feature_gate) { FactoryBot.create(:feature_gate) } describe "#save!" do - before do + it "doesn't merge feature gate assignments to ensure nobody piggybacks on a privileged identity to get into a closed feature gate" do + FactoryBot.create(:assignment, + visitor: visitor, + split: feature_gate, + variant: "true", + mixpanel_result: "success", + context: "context5") + + described_class.create!(superseded_visitor: visitor, superseding_visitor: existing_visitor) + + expect(Assignment.where(visitor: visitor, split: feature_gate)).to be_present + expect(Assignment.where(visitor: existing_visitor, split: feature_gate)).not_to be_present + end + + it "merges non-decided non-feature-gates to attempt to preserve user experience of experiments that span signup/auth" do + FactoryBot.create(:assignment, + visitor: visitor, + split: torque_split, + variant: "rear", + mixpanel_result: "success", + context: "context3") + + visitor_supersession = described_class.create!(superseded_visitor: visitor, superseding_visitor: existing_visitor) + + torque_split_assignment = existing_visitor.assignments.find_by!(split: torque_split, variant: "rear") + expect(torque_split_assignment.mixpanel_result).to eq nil + expect(torque_split_assignment.visitor_supersession).to eq visitor_supersession + expect(torque_split_assignment.context).to eq "visitor_supersession" + end + + it "doesn't merge decided non-feature-gates in a way that overrides the decision" do + FactoryBot.create(:assignment, + visitor: visitor, + split: decided_split, + variant: "bad_thing", + mixpanel_result: "success", + context: "context4") + decided_split.create_decision!(variant: "good_thing") + + described_class.create!(superseded_visitor: visitor, superseding_visitor: existing_visitor) + + expect(Assignment.for_presentation.where(visitor: visitor, split: decided_split)).not_to be_present + expect(Assignment.where(visitor: visitor, split: decided_split)).to be_present + + expect(Assignment.for_presentation.where(visitor: existing_visitor, split: decided_split)).not_to be_present + # expect(Assignment.where(visitor: existing_visitor, split: decided_split)).to HAVE_AS_YET_UNDEFINED_BEHAVIOR + end + + it "doesn't merge assignments that already exist on the target visitor" do FactoryBot.create(:assignment, visitor: visitor, split: banana_split, @@ -22,26 +72,13 @@ variant: "squishy", mixpanel_result: "success", context: "context2") - FactoryBot.create(:assignment, - visitor: visitor, - split: torque_split, - variant: "rear", - mixpanel_result: "success", - context: "context3") - end - it "merges assignments from the superseded visitor without overwriting existing assignments" do - visitor_supersession = described_class.create!(superseded_visitor: visitor, superseding_visitor: existing_visitor) + described_class.create!(superseded_visitor: visitor, superseding_visitor: existing_visitor) banana_split_assignment = existing_visitor.assignments.find_by!(split: banana_split, variant: "squishy") expect(banana_split_assignment.mixpanel_result).to eq "success" expect(banana_split_assignment.visitor_supersession).to eq nil expect(banana_split_assignment.context).to eq "context2" - - torque_split_assignment = existing_visitor.assignments.find_by!(split: torque_split, variant: "rear") - expect(torque_split_assignment.mixpanel_result).to eq nil - expect(torque_split_assignment.visitor_supersession).to eq visitor_supersession - expect(torque_split_assignment.context).to eq "visitor_supersession" end end end From 40104d47838c4b2121f046627815f17ed10bce1c Mon Sep 17 00:00:00 2001 From: John Mileham Date: Sun, 5 May 2019 15:55:32 -0400 Subject: [PATCH 2/2] Eliminate naming validations now that clients must run headless --- app/models/split.rb | 12 ------------ spec/models/split_spec.rb | 12 ------------ spec/models/split_upsert_spec.rb | 4 ++-- 3 files changed, 2 insertions(+), 26 deletions(-) diff --git a/app/models/split.rb b/app/models/split.rb index 77aabfe0..f4905b51 100644 --- a/app/models/split.rb +++ b/app/models/split.rb @@ -10,8 +10,6 @@ class Split < ActiveRecord::Base validates :registry, presence: true validate :name_must_be_snake_case - validate :name_must_not_include_new - validate :name_must_not_end_with_test validate :name_must_only_be_prefixed_with_app_name validate :variants_must_be_snake_case validate :registry_weights_must_sum_to_100 @@ -178,16 +176,6 @@ def name_must_be_snake_case errors.add(:name, "must be snake_case: #{name.inspect}") if name_not_underscored? end - def name_must_not_include_new - errors.add(:name, <<-ERROR_MESSAGE) if name_contains_new? - should not contain the ambiguous word 'new'. If expressing timing, refer to absolute time like 'late_2015'. If expressing creation use 'create'. - ERROR_MESSAGE - end - - def name_must_not_end_with_test - errors.add(:name, "should not end with 'test', as it is redundant. All splits are testable.") if name_ends_with_test? - end - def name_must_only_be_prefixed_with_app_name return if name.nil? || !name_changed? diff --git a/spec/models/split_spec.rb b/spec/models/split_spec.rb index dd2fccfb..19dae867 100644 --- a/spec/models/split_spec.rb +++ b/spec/models/split_spec.rb @@ -38,18 +38,6 @@ expect(subject.errors[:name].first).to include("snake_case") end - it "rejects new" do - subject.name = 'my_new_foo' - expect(subject).not_to be_valid - expect(subject.errors[:name].first).to include("absolute time") - end - - it "rejects ending in test" do - subject.name = 'my_foo_test' - expect(subject).not_to be_valid - expect(subject.errors[:name].first).to include("redundant") - end - it "rejects prefixed with other-than-app-name" do subject.name = 'bla.baz' expect(subject).not_to be_valid diff --git a/spec/models/split_upsert_spec.rb b/spec/models/split_upsert_spec.rb index fb91970f..30c21bae 100644 --- a/spec/models/split_upsert_spec.rb +++ b/spec/models/split_upsert_spec.rb @@ -55,9 +55,9 @@ end it 'delegates validation errors to split' do - split_upsert = described_class.new(app: default_app, name: "bad_test", weighting_registry: { badBadBad: 100 }) + split_upsert = described_class.new(app: default_app, name: "bump.bad_extremely_bad", weighting_registry: { badBadBad: 100 }) expect(split_upsert.save).to eq false - expect(split_upsert.errors[:name].first).to include("redundant") + expect(split_upsert.errors[:name].first).to include("prefixed") expect(split_upsert.errors[:weighting_registry].first).to include("snake_case") end