fixup! Validation of display order, default setting callbacks.

4 files changed, 52 insertions, 47 deletions

diff --git a/app/models/public_body_category_link.rb b/app/models/public_body_category_link.rb
index e5a0d7f25..d1ea5bdbb 100644
--- a/app/models/public_body_category_link.rb
+++ b/app/models/public_body_category_link.rb
@@ -17,11 +17,11 @@ class PublicBodyCategoryLink < ActiveRecord::Base
 
     before_validation :on => :create do
         unless self.category_display_order
-            self.category_display_order = PublicBodyCategoryLink.next_display_order(self.public_body_heading_id)
+            self.category_display_order = PublicBodyCategoryLink.next_display_order(public_body_heading_id)
         end
     end
 
-    def PublicBodyCategoryLink.next_display_order(heading_id)
+    def self.next_display_order(heading_id)
         if last = where(:public_body_heading_id => heading_id).order(:category_display_order).last
             last.category_display_order + 1
         else
diff --git a/app/models/public_body_heading.rb b/app/models/public_body_heading.rb
index e675162f8..bbed55b29 100644
--- a/app/models/public_body_heading.rb
+++ b/app/models/public_body_heading.rb
@@ -58,7 +58,7 @@ class PublicBodyHeading < ActiveRecord::Base
         end
     end
 
-    def PublicBodyHeading.next_display_order
+    def self.next_display_order
         if max = maximum(:display_order)
             max + 1
         else
diff --git a/spec/models/public_body_category_link_spec.rb b/spec/models/public_body_category_link_spec.rb
index 105f97864..7108f124b 100644
--- a/spec/models/public_body_category_link_spec.rb
+++ b/spec/models/public_body_category_link_spec.rb
@@ -12,10 +12,10 @@ require 'spec_helper'
 describe PublicBodyHeading, 'when validating' do
 
     it 'should set a default display order based on the next available display order' do
-        heading_with_no_categories = FactoryGirl.create(:heading_with_no_categories)
-        lonely_category = FactoryGirl.create(:lonely_category)
-        category_link = PublicBodyCategoryLink.new(:heading => heading_with_no_categories,
-                                                   :category_link => lonely_category)
+        heading = FactoryGirl.create(:public_body_heading)
+        category = FactoryGirl.create(:public_body_category)
+        category_link = PublicBodyCategoryLink.new(:public_body_heading => heading,
+                                                   :public_body_category => category)
         category_link.valid?
         category_link.category_display_order.should == PublicBodyCategoryLink.next_display_order(heading_with_no_categories)
     end
@@ -30,8 +30,12 @@ describe PublicBodyCategoryLink, 'when setting a category display order' do
     end
 
     it 'should return one more than the highest display order if there are public body headings' do
-        silly_heading = FactoryGirl.create(:silly_heading)
-        PublicBodyCategoryLink.next_display_order(silly_heading).should == 2
+        heading = FactoryGirl.create(:public_body_heading)
+        category = FactoryGirl.create(:public_body_category)
+        category_link = PublicBodyCategoryLink.create(:public_body_heading_id => heading.id,
+                                                      :public_body_category_id => category.id)
+
+        PublicBodyCategoryLink.next_display_order(heading).should == 1
     end
 
 end
diff --git a/spec/models/public_body_heading_spec.rb b/spec/models/public_body_heading_spec.rb
index 3ca01c648..5e581e713 100644
--- a/spec/models/public_body_heading_spec.rb
+++ b/spec/models/public_body_heading_spec.rb
@@ -10,54 +10,55 @@
 
 require 'spec_helper'
 
-describe PublicBodyHeading, 'when loading the data' do
+describe PublicBodyHeading do
 
-    before do
-        load_test_categories
-    end
-
-    it 'should use the display_order field to preserve the original data order' do
-        headings = PublicBodyHeading.all
-        headings[0].name.should eq 'Local and regional'
-        headings[0].display_order.should eq 1
-        headings[1].name.should eq 'Miscellaneous'
-        headings[1].display_order.should eq 2
-    end
+    context 'when loading the data' do
 
-end
+        before do
+            load_test_categories
+        end
 
-describe PublicBodyHeading, 'when validating' do
+        it 'should use the display_order field to preserve the original data order' do
+            headings = PublicBodyHeading.all
+            headings[0].name.should eq 'Local and regional'
+            headings[0].display_order.should eq 1
+            headings[1].name.should eq 'Miscellaneous'
+            headings[1].display_order.should eq 2
+        end
 
-    it 'should require a name' do
-        heading = PublicBodyHeading.new
-        heading.should_not be_valid
-        heading.errors[:name].should == ["Name can't be blank"]
     end
 
-    it 'should require a unique name' do
-        heading = FactoryGirl.create(:silly_heading)
-        new_heading = PublicBodyHeading.new(:name => heading.name)
-        new_heading.should_not be_valid
-        new_heading.errors[:name].should == ["Name is already taken"]
-    end
+    context 'when validating' do
 
-    it 'should set a default display order based on the next available display order' do
-        heading = PublicBodyHeading.new
-        heading.valid?
-        heading.display_order.should == PublicBodyHeading.next_display_order
-    end
+        it 'should require a name' do
+            heading = PublicBodyHeading.new
+            heading.should_not be_valid
+            heading.errors[:name].should == ["Name can't be blank"]
+        end
 
-end
-
-describe PublicBodyHeading, 'when setting a display order' do
+        it 'should require a unique name' do
+            heading = FactoryGirl.create(:silly_heading)
+            new_heading = PublicBodyHeading.new(:name => heading.name)
+            new_heading.should_not be_valid
+            new_heading.errors[:name].should == ["Name is already taken"]
+        end
 
-    it 'should return 0 if there are no public body headings' do
-        PublicBodyHeading.next_display_order.should == 0
+        it 'should set a default display order based on the next available display order' do
+            heading = PublicBodyHeading.new
+            heading.valid?
+            heading.display_order.should == PublicBodyHeading.next_display_order
+        end
     end
 
-    it 'should return one more than the highest display order if there are public body headings' do
-        heading = FactoryGirl.create(:popular_heading)
-        PublicBodyHeading.next_display_order.should == 2
-    end
+    context 'when setting a display order' do
 
+        it 'should return 0 if there are no public body headings' do
+            PublicBodyHeading.next_display_order.should == 0
+        end
+
+        it 'should return one more than the highest display order if there are public body headings' do
+            heading = FactoryGirl.create(:popular_heading)
+            PublicBodyHeading.next_display_order.should == 2
+        end
+    end
 end