Jmileham/relax naming validations

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

@@ -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
@@ -77,6 +75,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
@@ -176,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?
 

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/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

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
 

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