Fix to_create so it works when defined in a trait

lib/factory_girl.rb

@@ -30,6 +30,7 @@ require 'factory_girl/attribute_list'
 require 'factory_girl/trait'
 require 'factory_girl/aliases'
 require 'factory_girl/definition'
+require 'factory_girl/definition_list'
 require 'factory_girl/definition_proxy'
 require 'factory_girl/syntax'
 require 'factory_girl/syntax_runner'

lib/factory_girl/definition.rb

@@ -7,7 +7,7 @@ module FactoryGirl
       @declarations      = DeclarationList.new(name)
       @callbacks         = []
       @defined_traits    = []
-      @to_create         = default_to_create
+      @to_create         = nil
       @base_traits       = base_traits
       @additional_traits = []
       @constructor       = default_constructor
@@ -23,8 +23,10 @@ module FactoryGirl
       attributes
     end
 
-    def processing_order
-      base_traits + [self] + additional_traits
+    def definition_list
+      DefinitionList.new(
+        base_traits.map(&:definition) + [self] + additional_traits.map(&:definition)
+      )
     end
 
     def overridable
@@ -33,6 +35,10 @@ module FactoryGirl
     end
 
     def inherit_traits(new_traits)
+      @base_traits += new_traits
+    end
+
+    def append_traits(new_traits)
       @additional_traits += new_traits
     end
 
@@ -40,6 +46,10 @@ module FactoryGirl
       @callbacks << callback
     end
 
+    def compiled_to_create
+      definition_list.to_create
+    end
+
     def to_create(&block)
       if block_given?
         @to_create = block
@@ -60,20 +70,12 @@ module FactoryGirl
       @constructor != default_constructor
     end
 
-    def custom_to_create?
-      @to_create != default_to_create
-    end
-
     private
 
     def default_constructor
       @default_constructor ||= -> { new }
     end
 
-    def default_to_create
-      @default_to_create ||= ->(instance) { instance.save! }
-    end
-
     def base_traits
       @base_traits.map { |name| trait_by_name(name) }
     end

lib/factory_girl/definition_list.rb

@@ -0,0 +1,27 @@
+module FactoryGirl
+  class DefinitionList
+    include Enumerable
+
+    def initialize(definitions = [])
+      @definitions = definitions
+    end
+
+    def each(&block)
+      @definitions.each &block
+    end
+
+    def callbacks
+      map(&:callbacks).flatten
+    end
+
+    def attributes
+      map {|definition| definition.attributes.to_a }.flatten
+    end
+
+    def to_create
+      map(&:to_create).compact.last
+    end
+
+    delegate :[], :==, to: :@definitions
+  end
+end

lib/factory_girl/evaluation.rb

@@ -6,7 +6,7 @@ module FactoryGirl
 
     def initialize(attribute_assigner, to_create)
       @attribute_assigner = attribute_assigner
-      @to_create = to_create
+      @to_create = to_create || ->(instance) { instance.save! }
     end
 
     delegate :object, :hash, to: :@attribute_assigner

lib/factory_girl/factory.rb

@@ -17,7 +17,7 @@ module FactoryGirl
     end
 
     delegate :add_callback, :declare_attribute, :to_create, :define_trait,
-             :defined_traits, :inherit_traits, :processing_order, to: :@definition
+             :defined_traits, :inherit_traits, :append_traits, :definition_list, to: :@definition
 
     def build_class
       @build_class ||= if class_name.is_a? Class
@@ -36,7 +36,7 @@ module FactoryGirl
       evaluator = evaluator_class.new(build_class, strategy, overrides.symbolize_keys)
       attribute_assigner = AttributeAssigner.new(evaluator, build_class, &constructor)
 
-      evaluation = Evaluation.new(attribute_assigner, to_create)
+      evaluation = Evaluation.new(attribute_assigner, compiled_to_create)
       evaluation.add_observer(CallbacksObserver.new(callbacks, evaluator))
 
       strategy.result(evaluation).tap(&block)
@@ -90,12 +90,16 @@ module FactoryGirl
 
     def with_traits(traits)
       self.clone.tap do |factory_with_traits|
-        factory_with_traits.inherit_traits traits
+        factory_with_traits.append_traits traits
       end
     end
 
     protected
 
+    def compiled_to_create
+      @definition.compiled_to_create || parent.compiled_to_create
+    end
+
     def class_name
       @class_name || parent.class_name || name
     end
@@ -107,14 +111,12 @@ module FactoryGirl
     def attributes
       compile
       AttributeList.new(@name).tap do |list|
-        processing_order.each do |factory|
-          list.apply_attributes factory.attributes
-        end
+        list.apply_attributes definition_list.attributes
       end
     end
 
     def callbacks
-      parent.callbacks + processing_order.map {|factory| factory.callbacks }.flatten
+      parent.callbacks + definition_list.callbacks
     end
 
     def constructor

lib/factory_girl/null_factory.rb

@@ -7,7 +7,7 @@ module FactoryGirl
       @definition = Definition.new
     end
 
-    delegate :defined_traits, :callbacks, :attributes, :constructor, to: :definition
+    delegate :defined_traits, :callbacks, :attributes, :constructor, :compiled_to_create, to: :definition
 
     def compile; end
     def class_name; end

lib/factory_girl/trait.rb

@@ -1,7 +1,7 @@
 module FactoryGirl
   # @api private
   class Trait
-    attr_reader :name
+    attr_reader :name, :definition
 
     def initialize(name, &block)
       @name = name

spec/acceptance/traits_spec.rb

@@ -419,3 +419,71 @@ describe "making sure the factory is properly compiled the first time we want to
     user.role.should == 'admin'
   end
 end
+
+describe "traits with other than attributes or callbacks defined" do
+  before do
+    define_model("User", name: :string)
+
+    FactoryGirl.define do
+      factory :user do
+        trait :with_to_create do
+          to_create {|instance| instance.name = "to_create" }
+        end
+
+        factory :sub_user do
+          to_create {|instance| instance.name = "sub" }
+
+          factory :child_user
+        end
+
+        factory :sub_user_with_trait do
+          with_to_create
+
+          factory :child_user_with_trait
+        end
+
+        factory :sub_user_with_trait_and_override do
+          with_to_create
+          to_create {|instance| instance.name = "sub with trait and override" }
+
+          factory :child_user_with_trait_and_override
+        end
+      end
+    end
+  end
+
+  it "can apply to_create from traits" do
+    FactoryGirl.create(:user, :with_to_create).name.should == "to_create"
+  end
+
+  it "can apply to_create from the definition" do
+    FactoryGirl.create(:sub_user).name.should == "sub"
+    FactoryGirl.create(:child_user).name.should == "sub"
+  end
+
+  it "gives additional traits higher priority than to_create from the definition" do
+    FactoryGirl.create(:sub_user, :with_to_create).name.should == "to_create"
+    FactoryGirl.create(:child_user, :with_to_create).name.should == "to_create"
+  end
+
+  it "gives base traits normal priority" do
+    FactoryGirl.create(:sub_user_with_trait).name.should == "to_create"
+    FactoryGirl.create(:child_user_with_trait).name.should == "to_create"
+  end
+
+  it "gives base traits lower priority than overrides" do
+    FactoryGirl.create(:sub_user_with_trait_and_override).name.should == "sub with trait and override"
+    FactoryGirl.create(:child_user_with_trait_and_override).name.should == "sub with trait and override"
+  end
+
+  it "gives additional traits higher priority than base traits and factory definition" do
+    FactoryGirl.define do
+      trait :overridden do
+        to_create {|instance| instance.name = "completely overridden" }
+      end
+    end
+
+    FactoryGirl.create(:sub_user_with_trait_and_override, :overridden).name.should == "completely overridden"
+    FactoryGirl.create(:child_user_with_trait_and_override, :overridden).name.should == "completely overridden"
+  end
+end

spec/factory_girl/definition_spec.rb

@@ -49,13 +49,7 @@ describe FactoryGirl::Definition, "adding callbacks" do
 end
 
 describe FactoryGirl::Definition, "#to_create" do
-  its(:to_create) { should be_a(Proc) }
-
-  it "calls save! on the object when run" do
-    instance = stub("model instance", :save! => true)
-    subject.to_create[instance]
-    instance.should have_received(:save!).once
-  end
+  its(:to_create) { should be_nil }
 
   it "returns the assigned value when given a block" do
     block = proc { nil }
@@ -64,9 +58,11 @@ describe FactoryGirl::Definition, "#to_create" do
   end
 end
 
-describe FactoryGirl::Definition, "#processing_order" do
+describe FactoryGirl::Definition, "#definition_list" do
   let(:female_trait) { FactoryGirl::Trait.new(:female) }
   let(:admin_trait)  { FactoryGirl::Trait.new(:admin) }
+  let(:female_definition) { female_trait.definition }
+  let(:admin_definition) { admin_trait.definition }
 
   before do
     subject.define_trait(female_trait)
@@ -75,17 +71,22 @@ describe FactoryGirl::Definition, "#processing_order" do
 
   context "without base traits" do
     it "returns the definition without any traits" do
-      subject.processing_order.should == [subject]
+      subject.definition_list.should == [subject]
     end
 
-    it "finds the correct traits after inheriting" do
+    it "finds the correct definitions after appending" do
+      subject.append_traits([:female])
+      subject.definition_list.should == [subject, female_definition]
+    end
+
+    it "finds the correct definitions after inheriting" do
       subject.inherit_traits([:female])
-      subject.processing_order.should == [subject, female_trait]
+      subject.definition_list.should == [female_definition, subject]
     end
 
     it "looks for the trait on FactoryGirl" do
-      subject.inherit_traits([:female, :admin])
-      subject.processing_order.should == [subject, female_trait, admin_trait]
+      subject.append_traits([:female, :admin])
+      subject.definition_list.should == [subject, female_definition, admin_definition]
     end
   end
 
@@ -93,12 +94,17 @@ describe FactoryGirl::Definition, "#processing_order" do
     subject { FactoryGirl::Definition.new("my definition", [:female]) }
 
     it "returns the base traits and definition" do
-      subject.processing_order.should == [female_trait, subject]
+      subject.definition_list.should == [female_definition, subject]
     end
 
-    it "finds the correct traits after inheriting" do
+    it "finds the correct definitions after appending" do
+      subject.append_traits([:admin])
+      subject.definition_list.should == [female_definition, subject, admin_definition]
+    end
+
+    it "finds the correct definitions after inheriting" do
       subject.inherit_traits([:admin])
-      subject.processing_order.should == [female_trait, subject, admin_trait]
+      subject.definition_list.should == [female_definition, admin_definition, subject]
     end
   end
 end

spec/factory_girl/factory_spec.rb

@@ -262,18 +262,20 @@ describe FactoryGirl::Factory, "#with_traits" do
   subject            { FactoryGirl::Factory.new(:user) }
   let(:admin_trait)  { FactoryGirl::Trait.new(:admin) }
   let(:female_trait) { FactoryGirl::Trait.new(:female) }
+  let(:admin_definition) { admin_trait.definition }
+  let(:female_definition) { female_trait.definition }
 
   before do
     FactoryGirl.register_trait(admin_trait)
     FactoryGirl.register_trait(female_trait)
   end
 
-  it "returns a factory with the correct traits" do
-    subject.with_traits([:admin, :female]).processing_order[1, 2].should == [admin_trait, female_trait]
+  it "returns a factory with the correct definitions" do
+    subject.with_traits([:admin, :female]).definition_list[1, 2].should == [admin_definition, female_definition]
   end
 
   it "doesn't modify the original factory's processing order" do
     subject.with_traits([:admin, :female])
-    subject.processing_order.should == [subject.definition]
+    subject.definition_list.should == [subject.definition]
   end
 end