Fix various trait bugs so traits can be used within each other

The previous implementation of trait handling within the Definition
didn't account for when implicit traits were used within other traits.
This is useful if you have two different traits, but one depends on
another; for example, a refunded order and a completed order could both
have the attribute `completed_at` set, but refunded would additionally
have `refunded_at` set:

    FactoryGirl.define do
      factory :order do
        trait :completed do
          completed_at { 3.days.ago }
        end

        trait :refunded do
          completed
          refunded_at { 1.day.ago }
        end
      end
    end

This also tests to make sure that callbacks, custom constructors, and
creation overrides work correctly when implicit traits are used within
other traits.

Fixes #360

lib/factory_girl.rb

@@ -30,7 +30,6 @@ 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/declaration_list.rb

@@ -20,7 +20,7 @@ module FactoryGirl
       @overridable = true
     end
 
-    def attribute_list
+    def attributes
       AttributeList.new(@name).tap do |list|
         to_attributes.each do |attribute|
           list.define_attribute(attribute)

lib/factory_girl/definition.rb

@@ -1,7 +1,7 @@
 module FactoryGirl
   # @api private
   class Definition
-    attr_reader :callbacks, :defined_traits, :declarations, :constructor
+    attr_reader :defined_traits, :declarations
 
     def initialize(name = nil, base_traits = [])
       @declarations      = DeclarationList.new(name)
@@ -11,22 +11,48 @@ module FactoryGirl
       @base_traits       = base_traits
       @additional_traits = []
       @constructor       = nil
+      @attributes        = nil
+      @compiled          = false
     end
 
     delegate :declare_attribute, to: :declarations
 
     def attributes
-      @attributes ||= declarations.attribute_list
+      @attributes ||= AttributeList.new.tap do |attribute_list|
+        attribute_lists = aggregate_from_traits_and_self(:attributes) { declarations.attributes }
+        attribute_lists.each do |attributes|
+          attribute_list.apply_attributes attributes
+        end
+      end
+    end
+
+    def to_create(&block)
+      if block_given?
+        @to_create = block
+      else
+        aggregate_from_traits_and_self(:to_create) { @to_create }.last
+      end
+    end
+
+    def constructor
+      aggregate_from_traits_and_self(:constructor) { @constructor }.last
+    end
+
+    def callbacks
+      aggregate_from_traits_and_self(:callbacks) { @callbacks }
     end
 
     def compile
-      attributes
+      unless @compiled
-    end
+        declarations.attributes
 
-    def definition_list
+        defined_traits.each do |defined_trait|
-      DefinitionList.new(
+          base_traits.each       {|bt| bt.define_trait defined_trait }
-        base_traits.map(&:definition) + [self] + additional_traits.map(&:definition)
+          additional_traits.each {|bt| bt.define_trait defined_trait }
-      )
+        end
+
+        @compiled = true
+      end
     end
 
     def overridable
@@ -46,22 +72,6 @@ module FactoryGirl
       @callbacks << callback
     end
 
-    def compiled_to_create
-      definition_list.to_create
-    end
-
-    def compiled_constructor
-      definition_list.constructor
-    end
-
-    def to_create(&block)
-      if block_given?
-        @to_create = block
-      else
-        @to_create
-      end
-    end
-
     def skip_create
       @to_create = ->(instance) { }
     end
@@ -91,5 +101,26 @@ module FactoryGirl
     def trait_for(name)
       defined_traits.detect {|trait| trait.name == name }
     end
+
+    def initialize_copy(source)
+      super
+      @attributes = nil
+      @compiled   = false
+    end
+
+    def aggregate_from_traits_and_self(method_name, &block)
+      compile
+      [].tap do |list|
+        base_traits.each do |trait|
+          list << trait.send(method_name)
+        end
+
+        list << instance_exec(&block)
+
+        additional_traits.each do |trait|
+          list << trait.send(method_name)
+        end
+      end.flatten.compact
+    end
   end
 end

lib/factory_girl/definition_list.rb

@@ -1,31 +0,0 @@
-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
-
-    def constructor
-      map(&:constructor).compact.last
-    end
-
-    delegate :[], :==, to: :@definitions
-  end
-end

lib/factory_girl/factory.rb

@@ -17,7 +17,7 @@ module FactoryGirl
     end
 
     delegate :add_callback, :declare_attribute, :to_create, :define_trait, :constructor,
-             :defined_traits, :inherit_traits, :append_traits, :definition_list, to: :@definition
+             :defined_traits, :inherit_traits, :append_traits, to: :@definition
 
     def build_class
       @build_class ||= if class_name.is_a? Class
@@ -107,20 +107,20 @@ module FactoryGirl
     def attributes
       compile
       AttributeList.new(@name).tap do |list|
-        list.apply_attributes definition_list.attributes
+        list.apply_attributes definition.attributes
       end
     end
 
     def callbacks
-      parent.callbacks + definition_list.callbacks
+      parent.callbacks + definition.callbacks
     end
 
     def compiled_to_create
-      @definition.compiled_to_create || parent.compiled_to_create || FactoryGirl.to_create
+      @definition.to_create || parent.compiled_to_create || FactoryGirl.to_create
     end
 
     def compiled_constructor
-      @definition.compiled_constructor || parent.compiled_constructor || FactoryGirl.constructor
+      @definition.constructor || parent.compiled_constructor || FactoryGirl.constructor
     end
 
     private

lib/factory_girl/null_factory.rb

@@ -4,12 +4,14 @@ module FactoryGirl
     attr_reader :definition
 
     def initialize
-      @definition = Definition.new
+      @definition = Definition.new(:null_factory)
     end
 
     delegate :defined_traits, :callbacks, :attributes, :constructor,
-      :compiled_to_create, :compiled_constructor, to: :definition
+      :to_create, to: :definition
 
+    def compiled_to_create; to_create; end
+    def compiled_constructor; constructor; end
     def compile; end
     def class_name; end
     def evaluator_class; FactoryGirl::Evaluator; end

spec/acceptance/traits_spec.rb

@@ -563,3 +563,85 @@ describe "traits with initialize_with" do
     FactoryGirl.build(:child_user_with_trait_and_override, :overridden).name.should == "completely overridden"
   end
 end
+
+describe "nested implicit traits" do
+  before do
+    define_class("User") do
+      attr_accessor :gender, :role
+      attr_reader :name
+
+      def initialize(name)
+        @name = name
+      end
+    end
+  end
+
+  shared_examples_for "assigning data from traits" do
+    it "assigns the correct values" do
+      user = FactoryGirl.create(:user, :female_admin)
+      user.gender.should == "FEMALE"
+      user.role.should == "ADMIN"
+      user.name.should == "Jane Doe"
+    end
+  end
+
+  context "defined outside the factory" do
+    before do
+      FactoryGirl.define do
+        trait :female do
+          gender "female"
+          to_create {|instance| instance.gender = instance.gender.upcase }
+        end
+
+        trait :jane_doe do
+          initialize_with { new("Jane Doe") }
+        end
+
+        trait :admin do
+          role "admin"
+          after(:build) {|instance| instance.role = instance.role.upcase }
+        end
+
+        trait :female_admin do
+          female
+          admin
+          jane_doe
+        end
+
+        factory :user
+      end
+    end
+
+    it_should_behave_like "assigning data from traits"
+  end
+
+  context "defined inside the factory" do
+    before do
+      FactoryGirl.define do
+        factory :user do
+          trait :female do
+            gender "female"
+            to_create {|instance| instance.gender = instance.gender.upcase }
+          end
+
+          trait :jane_doe do
+            initialize_with { new("Jane Doe") }
+          end
+
+          trait :admin do
+            role "admin"
+            after(:build) {|instance| instance.role = instance.role.upcase }
+          end
+
+          trait :female_admin do
+            female
+            admin
+            jane_doe
+          end
+        end
+      end
+    end
+
+    it_should_behave_like "assigning data from traits"
+  end
+end

spec/factory_girl/declaration_list_spec.rb

@@ -1,6 +1,6 @@
 require "spec_helper"
 
-describe FactoryGirl::DeclarationList, "#attribute_list" do
+describe FactoryGirl::DeclarationList, "#attributes" do
   let(:static_attribute_1)  { stub("static attribute 1") }
   let(:static_attribute_2)  { stub("static attribute 2") }
   let(:dynamic_attribute_1) { stub("dynamic attribute 1") }
@@ -8,7 +8,7 @@ describe FactoryGirl::DeclarationList, "#attribute_list" do
   let(:dynamic_declaration) { stub("static declaration", to_attributes: [dynamic_attribute_1]) }
 
   it "returns an AttributeList" do
-    subject.attribute_list.should be_a(FactoryGirl::AttributeList)
+    subject.attributes.should be_a(FactoryGirl::AttributeList)
   end
 
   let(:attribute_list) { stub("attribute list", define_attribute: true) }
@@ -19,7 +19,7 @@ describe FactoryGirl::DeclarationList, "#attribute_list" do
     subject.declare_attribute(static_declaration)
     subject.declare_attribute(dynamic_declaration)
 
-    subject.attribute_list
+    subject.attributes
 
     attribute_list.should have_received(:define_attribute).with(static_attribute_1)
     attribute_list.should have_received(:define_attribute).with(static_attribute_2)
@@ -27,7 +27,7 @@ describe FactoryGirl::DeclarationList, "#attribute_list" do
   end
 
   it "creates a new attribute list upon every invocation" do
-    subject.attribute_list.should_not == subject.attribute_list
+    subject.attributes.should_not == subject.attributes
   end
 end
 

spec/factory_girl/definition_spec.rb

@@ -2,7 +2,6 @@ require "spec_helper"
 
 describe FactoryGirl::Definition do
   it { should delegate(:declare_attribute).to(:declarations) }
-  it { should delegate(:attributes).to(:declarations).as(:attribute_list) }
 end
 
 describe FactoryGirl::Definition, "with a name" do
@@ -38,8 +37,8 @@ describe FactoryGirl::Definition, "defining traits" do
 end
 
 describe FactoryGirl::Definition, "adding callbacks" do
-  let(:callback_1) { stub("callback") }
+  let(:callback_1) { "callback1" }
-  let(:callback_2) { stub("callback") }
+  let(:callback_2) { "callback2" }
 
   it "maintains a list of callbacks" do
     subject.add_callback(callback_1)
@@ -57,54 +56,3 @@ describe FactoryGirl::Definition, "#to_create" do
     subject.to_create.should == block
   end
 end
-
-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)
-    FactoryGirl.stubs(trait_by_name: admin_trait)
-  end
-
-  context "without base traits" do
-    it "returns the definition without any traits" do
-      subject.definition_list.should == [subject]
-    end
-
-    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.definition_list.should == [female_definition, subject]
-    end
-
-    it "looks for the trait on FactoryGirl" do
-      subject.append_traits([:female, :admin])
-      subject.definition_list.should == [subject, female_definition, admin_definition]
-    end
-  end
-
-  context "with base traits" do
-    subject { FactoryGirl::Definition.new("my definition", [:female]) }
-
-    it "returns the base traits and definition" do
-      subject.definition_list.should == [female_definition, subject]
-    end
-
-    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.definition_list.should == [female_definition, admin_definition, subject]
-    end
-  end
-end

spec/factory_girl/factory_spec.rb

@@ -257,25 +257,3 @@ describe FactoryGirl::Factory, "running a factory" do
     block_run.should == "changed"
   end
 end
-
-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 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.definition_list.should == [subject.definition]
-  end
-end