Don't merge feature gate assignments when superseding visitors (#115)

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

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

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

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