Add display order to public body categories and headings

8 files changed, 208 insertions, 60 deletions

diff --git a/app/controllers/admin_public_body_category_controller.rb b/app/controllers/admin_public_body_category_controller.rb
index 221d8e4ad..f814763ad 100644
--- a/app/controllers/admin_public_body_category_controller.rb
+++ b/app/controllers/admin_public_body_category_controller.rb
@@ -26,10 +26,27 @@ class AdminPublicBodyCategoryController < AdminController
             else
                 if params[:headings]
                     heading_ids = params[:headings].values
+                    removed_headings = @category.public_body_heading_ids - heading_ids
+                    added_headings = heading_ids - @category.public_body_heading_ids
+
+                    unless removed_headings.empty?
+                        # remove the link objects
+                        deleted_links = PublicBodyCategoryLink.where(
+                            :public_body_category_id => @category.id,
+                            :public_body_heading_id => [removed_headings]
+                        )
+                        deleted_links.delete_all
+
+                        #fix the category object
+                        @category.public_body_heading_ids = heading_ids
+                    end
+
+                    added_headings.each do |heading_id|
+                        @category.add_to_heading(PublicBodyHeading.find(heading_id))
+                    end
                 end
 
                 if @category.update_attributes(params[:public_body_category])
-                    @category.public_body_heading_ids = heading_ids
                     flash[:notice] = 'Category was successfully updated.'
                 end
             end
diff --git a/app/models/public_body_category.rb b/app/models/public_body_category.rb
index cb15f67e6..24b3c89b4 100644
--- a/app/models/public_body_category.rb
+++ b/app/models/public_body_category.rb
@@ -6,14 +6,17 @@
 #  title         :text           not null
 #  category_tag  :text           not null
 #  description   :text           not null
+#  display_order :integer
 #
 
 require 'forwardable'
 
 class PublicBodyCategory < ActiveRecord::Base
-    attr_accessible :locale, :category_tag, :title, :description, :translated_versions
+    attr_accessible :locale, :category_tag, :title, :description,
+                    :translated_versions, :display_order
 
-    has_and_belongs_to_many :public_body_headings
+    has_many :public_body_category_links
+    has_many :public_body_headings, :through => :public_body_category_links
 
     translates :title, :description
     validates_uniqueness_of :category_tag, :message => N_('Tag is already taken')
@@ -46,7 +49,7 @@ class PublicBodyCategory < ActiveRecord::Base
         sql = %Q| SELECT * FROM public_body_categories pbc
                   WHERE pbc.id NOT IN (
                       SELECT public_body_category_id AS id
-                      FROM public_body_categories_public_body_headings
+                      FROM public_body_category_links
                   ) |
         PublicBodyCategory.find_by_sql(sql)
     end
@@ -54,6 +57,7 @@ class PublicBodyCategory < ActiveRecord::Base
     # Called from the data files themselves
     def self.add(locale, categories)
         @heading = nil
+        @heading_order = 0
         categories.each do |category|
             if category.is_a?(Array)
                 #categories
@@ -75,13 +79,15 @@ class PublicBodyCategory < ActiveRecord::Base
                             pb_category.save
                         end
                     end
-                    pb_category.public_body_headings << @heading
+
+                    pb_category.add_to_heading(@heading)
                 else
                     I18n.with_locale(locale) do
                         pb_category.title = category[1]
                         pb_category.description = category[2]
                         pb_category.save
                     end
+                    pb_category.add_to_heading(@heading)
                 end
             else
                 #headings
@@ -94,13 +100,43 @@ class PublicBodyCategory < ActiveRecord::Base
                     end
                 else
                     I18n.with_locale(locale) do
-                        @heading = PublicBodyHeading.create(:name => category)
+                        last_heading = PublicBodyHeading.last
+                        if last_heading
+                            @heading_order = last_heading.display_order + 1
+                        else
+                            @heading_order = 1
+                        end
+                        @heading = PublicBodyHeading.create(:name => category, :display_order => @heading_order)
                     end
                 end
             end
         end
     end
 
+    def add_to_heading(heading)
+        if self.public_body_headings.include?(heading)
+            # we already have this, stop
+            return
+        end
+
+        # find the last display_order for this heading
+        last_link = PublicBodyCategoryLink.where(
+            :public_body_heading_id => heading.id
+        ).order(:category_display_order).last
+
+        if last_link
+            display_order = last_link.category_display_order + 1
+        else
+            display_order = 1
+        end
+
+        heading_link = PublicBodyCategoryLink.create(
+            :public_body_category_id => self.id,
+            :public_body_heading_id => heading.id,
+            :category_display_order => display_order
+        )
+    end
+
     # Convenience methods for creating/editing translations via forms
     def find_translation_by_locale(locale)
         self.translations.find_by_locale(locale)
diff --git a/app/models/public_body_category_link.rb b/app/models/public_body_category_link.rb
new file mode 100644
index 000000000..bd4fe541a
--- /dev/null
+++ b/app/models/public_body_category_link.rb
@@ -0,0 +1,15 @@
+# == Schema Information
+#
+# Table name: public_body_category_link
+#
+#  public_body_category_id       :integer        not null
+#  public_body_heading_id        :integer        not null
+#  category_display_order        :integer
+#
+
+class PublicBodyCategoryLink < ActiveRecord::Base
+    attr_accessible :public_body_category_id, :public_body_heading_id, :category_display_order
+
+    belongs_to :public_body_category
+    belongs_to :public_body_heading
+end
\ No newline at end of file
diff --git a/app/models/public_body_heading.rb b/app/models/public_body_heading.rb
index 6c7d645da..818940341 100644
--- a/app/models/public_body_heading.rb
+++ b/app/models/public_body_heading.rb
@@ -4,10 +4,15 @@
 #
 #  id            :integer        not null, primary key
 #  name          :text           not null
+#  display_order :integer
 #
 
 class PublicBodyHeading < ActiveRecord::Base
-    has_and_belongs_to_many :public_body_categories
+    attr_accessible :name, :display_order, :translated_versions
+
+    has_many :public_body_category_links
+    has_many :public_body_categories, :order => :category_display_order, :through => :public_body_category_links
+    default_scope order('display_order ASC')
 
     translates :name
 
diff --git a/db/migrate/20140804120601_add_display_order_to_categories_and_headings.rb b/db/migrate/20140804120601_add_display_order_to_categories_and_headings.rb
new file mode 100644
index 000000000..60ac5ec5e
--- /dev/null
+++ b/db/migrate/20140804120601_add_display_order_to_categories_and_headings.rb
@@ -0,0 +1,16 @@
+class AddDisplayOrderToCategoriesAndHeadings < ActiveRecord::Migration
+  def up
+      add_column :public_body_categories_public_body_headings, :category_display_order, :integer
+
+      rename_table :public_body_categories_public_body_headings, :public_body_category_links
+      add_column :public_body_category_links, :id, :primary_key
+      add_index :public_body_category_links, [:public_body_category_id, :public_body_heading_id], :name => "index_public_body_category_links_on_join_ids", :primary => true
+  end
+
+  def down
+      remove_index :public_body_category_links, :name => "index_public_body_category_links_on_join_ids"
+      remove_column :public_body_category_links, :category_display_order
+      remove_column :public_body_category_links, :id
+      rename_table :public_body_category_links, :public_body_categories_public_body_headings
+  end
+end
diff --git a/spec/models/public_body_category_link_spec.rb b/spec/models/public_body_category_link_spec.rb
new file mode 100644
index 000000000..9fce062a3
--- /dev/null
+++ b/spec/models/public_body_category_link_spec.rb
@@ -0,0 +1,13 @@
+# == Schema Information
+#
+# Table name: public_body_category_link
+#
+#  public_body_category_id       :integer        not null
+#  public_body_heading_id        :integer        not null
+#  category_display_order        :integer
+#
+
+require 'spec_helper'
+
+describe PublicBodyCategoryLink do
+end
\ No newline at end of file
diff --git a/spec/models/public_body_category_spec.rb b/spec/models/public_body_category_spec.rb
index 8c0adbcc6..d892c8bda 100644
--- a/spec/models/public_body_category_spec.rb
+++ b/spec/models/public_body_category_spec.rb
@@ -7,81 +7,113 @@
 #  title         :text           not null
 #  category_tag  :text           not null
 #  description   :text           not null
+#  display_order :integer
 #
 
 require File.expand_path(File.dirname(__FILE__) + '/../spec_helper')
 
 describe PublicBodyCategory do
+    describe 'when loading the data' do
+        it 'should use the display_order field to preserve the original data order' do
+            PublicBodyCategories.add(:en, [
+                "Local and regional",
+                    [ "local_council", "Local councils", "a local council" ],
+                "Miscellaneous",
+                    [ "other", "Miscellaneous", "miscellaneous" ],
+                    [ "aardvark", "Aardvark", "daft test"],])
 
-    before do
-        load_test_categories
-    end
+            headings = PublicBodyHeading.all
+            cat_group1 = headings[0].public_body_categories
+            cat_group1.count.should eq 1
+            cat_group1[0].title.should eq "Local councils"
+
+            cat_group2 = headings[1].public_body_categories
+            cat_group2.count.should eq 2
+            cat_group2[0].title.should eq "Miscellaneous"
+            cat_group2[0].public_body_category_links.where(
+                :public_body_heading_id => headings[1].id).
+                    first.
+                    category_display_order.should eq 1
 
-    describe 'when asked for categories with headings' do
-        it 'should return a list of headings as plain strings, each followed by n tag specifications as
-            lists in the form:
-            ["tag_to_use_as_category", "Sub category title", "Instance description"]' do
-            expected_categories = ["Local and regional", ["local_council",
-                                                          "Local councils",
-                                                          "a local council"],
-                                   "Miscellaneous", ["other",
-                                                     "Miscellaneous",
-                                                     "miscellaneous"]]
-            PublicBodyCategory::get().with_headings().should == expected_categories
+            cat_group2[1].title.should eq "Aardvark"
+            cat_group2[1].public_body_category_links.where(
+                :public_body_heading_id => headings[1].id).
+                    first.
+                    category_display_order.should eq 2
         end
     end
 
-    describe 'when asked for headings' do
-        it 'should return a list of headings' do
-            PublicBodyCategory::get().headings().should == ['Local and regional', 'Miscellaneous']
+    context "requesting data" do
+        before do
+            load_test_categories
         end
 
-        it 'should call load_categories if categories are not already loaded' do
-            PublicBodyCategory.stub!(:count).and_return(0)
-            PublicBodyCategory.should_receive(:load_categories)
-            PublicBodyCategory::get()
+        describe 'when asked for categories with headings' do
+            it 'should return a list of headings as plain strings, each followed by n tag specifications as
+                lists in the form:
+                ["tag_to_use_as_category", "Sub category title", "Instance description"]' do
+                expected_categories = ["Local and regional", ["local_council",
+                                                              "Local councils",
+                                                              "a local council"],
+                                       "Miscellaneous", ["other",
+                                                         "Miscellaneous",
+                                                         "miscellaneous"]]
+                PublicBodyCategory::get().with_headings().should == expected_categories
+            end
         end
-    end
 
-    describe 'when asked for tags by headings' do
-        it 'should return a hash of tags keyed by heading' do
-            PublicBodyCategory::get().by_heading().should == {'Local and regional' => ['local_council'],
-                                                                'Miscellaneous' => ['other']}
+        describe 'when asked for headings' do
+            it 'should return a list of headings' do
+                PublicBodyCategory::get().headings().should == ['Local and regional', 'Miscellaneous']
+            end
+
+            it 'should call load_categories if categories are not already loaded' do
+                PublicBodyCategory.stub!(:count).and_return(0)
+                PublicBodyCategory.should_receive(:load_categories)
+                PublicBodyCategory::get()
+            end
         end
-    end
 
-    describe 'when asked for categories with description' do
-        it 'should return a list of tag specifications as lists in the form:
-            ["tag_to_use_as_category", "Sub category title", "Instance description"]' do
-            expected_categories = [
-                                        ["local_council", "Local councils", "a local council"],
-                                        ["other", "Miscellaneous", "miscellaneous"]
-                                  ]
-            PublicBodyCategory::get().with_description().should == expected_categories
+        describe 'when asked for tags by headings' do
+            it 'should return a hash of tags keyed by heading' do
+                PublicBodyCategory::get().by_heading().should == {'Local and regional' => ['local_council'],
+                                                                    'Miscellaneous' => ['other']}
+            end
         end
-    end
 
-    describe 'when asked for tags' do
-        it 'should return a list of tags' do
-            PublicBodyCategory::get().tags().should == ["local_council", "other"]
+        describe 'when asked for categories with description' do
+            it 'should return a list of tag specifications as lists in the form:
+                ["tag_to_use_as_category", "Sub category title", "Instance description"]' do
+                expected_categories = [
+                                            ["local_council", "Local councils", "a local council"],
+                                            ["other", "Miscellaneous", "miscellaneous"]
+                                      ]
+                PublicBodyCategory::get().with_description().should == expected_categories
+            end
         end
-    end
 
-    describe 'when asked for categories by tag' do
-        it 'should return a hash of categories keyed by tag' do
-            PublicBodyCategory::get().by_tag().should == {
-                "local_council" => "Local councils",
-                "other" => "Miscellaneous"
-            }
+        describe 'when asked for tags' do
+            it 'should return a list of tags' do
+                PublicBodyCategory::get().tags().should == ["local_council", "other"]
+            end
+        end
+
+        describe 'when asked for categories by tag' do
+            it 'should return a hash of categories keyed by tag' do
+                PublicBodyCategory::get().by_tag().should == {
+                    "local_council" => "Local councils",
+                    "other" => "Miscellaneous"
+                }
+            end
         end
-    end
 
-    describe 'when asked for singular_by_tag' do
-        it 'should return a hash of category descriptions keyed by tag' do
-            PublicBodyCategory::get().singular_by_tag().should == {
-                "local_council" => "a local council",
-                "other" => "miscellaneous"
-            }
+        describe 'when asked for singular_by_tag' do
+            it 'should return a hash of category descriptions keyed by tag' do
+                PublicBodyCategory::get().singular_by_tag().should == {
+                    "local_council" => "a local council",
+                    "other" => "miscellaneous"
+                }
+            end
         end
     end
 end
\ No newline at end of file
diff --git a/spec/models/public_body_heading_spec.rb b/spec/models/public_body_heading_spec.rb
index 73b5167fb..ec5c3ad2c 100644
--- a/spec/models/public_body_heading_spec.rb
+++ b/spec/models/public_body_heading_spec.rb
@@ -5,9 +5,23 @@
 #  id            :integer        not null, primary key
 #  locale        :string
 #  name          :text           not null
+#  display_order :integer
 #
 
 require 'spec_helper'
 
 describe PublicBodyHeading do
+    before do
+        load_test_categories
+    end
+
+    describe 'when loading the data' 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
+    end
 end