Don't display duplicate public bodies with the fallback

This introduces some raw SQL statement for the fallback case, but we
can't see an easy way to avoid that in this case.

This commit also adds some tests that assert the sorting and
non-duplication properties of the listing.

Thanks to Louise Crow for working out the SQL expression for
falling back to the default locale.

4 files changed, 110 insertions, 30 deletions

diff --git a/Gemfile b/Gemfile
index b79cf3250..a32903e4d 100644
--- a/Gemfile
+++ b/Gemfile
@@ -54,6 +54,7 @@ group :test do
   gem 'fakeweb'
   gem 'coveralls', :require => false
   gem 'webrat'
+  gem 'nokogiri'
 end
 
 group :development do
diff --git a/Gemfile.lock b/Gemfile.lock
index 75d1ac4be..e0e846758 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -269,6 +269,7 @@ DEPENDENCIES
   net-http-local
   net-purge
   newrelic_rpm
+  nokogiri
   pg
   rack
   rails (= 3.1.12)
diff --git a/app/controllers/public_body_controller.rb b/app/controllers/public_body_controller.rb
index 335dddf65..518e6c77a 100644
--- a/app/controllers/public_body_controller.rb
+++ b/app/controllers/public_body_controller.rb
@@ -85,44 +85,45 @@ class PublicBodyController < ApplicationController
     def list
         long_cache
         # XXX move some of these tag SQL queries into has_tag_string.rb
-        @query = "%#{params[:public_body_query].nil? ? "" : params[:public_body_query]}%"
+
+        like_query = params[:public_body_query]
+        like_query = "" if like_query.nil?
+        like_query = "%#{like_query}%"
+
         @tag = params[:tag]
+
         @locale = self.locale_from_params
         underscore_locale = @locale.gsub '-', '_'
         underscore_default_locale = I18n.default_locale.to_s.gsub '-', '_'
-        locale_condition = "(upper(public_body_translations.name) LIKE upper(?)" \
-                           " OR upper(public_body_translations.notes) LIKE upper (?))" \
-                           " AND public_bodies.id <> #{PublicBody.internal_admin_body.id}"
-        condition_parameters = [@query, @query]
-        if AlaveteliConfiguration::public_body_list_fallback_to_default_locale
-            locale_condition += " AND (public_body_translations.locale = ? OR public_body_translations.locale = ?)"
-            condition_parameters.concat [underscore_locale, underscore_default_locale]
-        else
-            locale_condition += " AND public_body_translations.locale = ?"
-            condition_parameters.concat [underscore_locale]
-        end
+
+        where_condition = "public_bodies.id <> #{PublicBody.internal_admin_body.id}"
+        where_parameters = []
+
+        first_letter = false
+
+        base_tag_condition = " AND (SELECT count(*) FROM has_tag_string_tags" \
+            " WHERE has_tag_string_tags.model_id = public_bodies.id" \
+            " AND has_tag_string_tags.model = 'PublicBody'"
+
+        # Restrict the public bodies shown according to the tag
+        # parameter supplied in the URL:
         if @tag.nil? or @tag == "all"
             @tag = "all"
         elsif @tag == 'other'
             category_list = PublicBodyCategories::get().tags().map{|c| "'"+c+"'"}.join(",")
-            locale_condition += " AND (SELECT count(*) FROM has_tag_string_tags WHERE has_tag_string_tags.model_id = public_bodies.id" \
-                " AND has_tag_string_tags.model = 'PublicBody'" \
-                " AND has_tag_string_tags.name in (#{category_list})) = 0"
+            where_condition += base_tag_condition + " AND has_tag_string_tags.name in (#{category_list})) = 0"
         elsif @tag.size == 1
             @tag.upcase!
-            locale_condition += " AND public_body_translations.first_letter = ?"
-            condition_parameters.concat [@tag]
+            # The first letter queries have to be done on
+            # translations, so just indicate to add that later:
+            first_letter = true
         elsif @tag.include?(":")
             name, value = HasTagString::HasTagStringTag.split_tag_into_name_value(@tag)
-            locale_condition += " AND (SELECT count(*) FROM has_tag_string_tags WHERE has_tag_string_tags.model_id = public_bodies.id" \
-                " AND has_tag_string_tags.model = 'PublicBody'" \
-                " AND has_tag_string_tags.name = ? AND has_tag_string_tags.value = ?) > 0"
-            condition_parameters.concat [name, value]
+            where_condition += base_tag_condition + " AND has_tag_string_tags.name = ? AND has_tag_string_tags.value = ?) > 0"
+            where_parameters.concat [name, value]
         else
-            locale_condition += " AND (SELECT count(*) FROM has_tag_string_tags WHERE has_tag_string_tags.model_id = public_bodies.id" \
-                " AND has_tag_string_tags.model = 'PublicBody'" \
-                " AND has_tag_string_tags.name = ?) > 0"
-            condition_parameters.concat [@tag]
+            where_condition += base_tag_condition + " AND has_tag_string_tags.name = ?) > 0"
+            where_parameters.concat [@tag]
         end
 
         if @tag == "all"
@@ -137,11 +138,45 @@ class PublicBodyController < ApplicationController
                 @description = _("in the category ‘{{category_name}}’", :category_name=>category_name)
             end
         end
-        conditions = [locale_condition] + condition_parameters
+
         I18n.with_locale(@locale) do
-            @public_bodies = PublicBody.where(conditions).joins(:translations).order("public_body_translations.name").paginate(
-              :page => params[:page], :per_page => 100
-            )
+
+            if AlaveteliConfiguration::public_body_list_fallback_to_default_locale
+                # Unfortunately, when we might fall back to the
+                # default locale, this is a rather complex query:
+                query =  %Q{
+                    SELECT public_bodies.*, COALESCE(current_locale.name, default_locale.name) AS display_name
+                    FROM public_bodies
+                    LEFT OUTER JOIN public_body_translations as current_locale
+                        ON (public_bodies.id = current_locale.public_body_id
+                            AND current_locale.locale = ? AND #{get_public_body_list_translated_condition 'current_locale', first_letter})
+                    LEFT OUTER JOIN public_body_translations as default_locale
+                        ON (public_bodies.id = default_locale.public_body_id
+                            AND default_locale.locale = ? AND #{get_public_body_list_translated_condition 'default_locale', first_letter})
+                    WHERE #{where_condition} AND COALESCE(current_locale.name, default_locale.name) IS NOT NULL
+                    ORDER BY display_name}
+                sql = [query, underscore_locale, like_query, like_query]
+                sql.push @tag if first_letter
+                sql += [underscore_default_locale, like_query, like_query]
+                sql.push @tag if first_letter
+                sql += where_parameters
+                @public_bodies = PublicBody.paginate_by_sql(
+                    sql,
+                    :page => params[:page],
+                    :per_page => 100)
+            else
+                # The simpler case where we're just searching in the current locale:
+                where_condition = get_public_body_list_translated_condition('public_body_translations', first_letter, true) +
+                    ' AND ' + where_condition
+                where_sql = [where_condition, like_query, like_query]
+                where_sql.push @tag if first_letter
+                where_sql += [underscore_locale] + where_parameters
+                @public_bodies = PublicBody.where(where_sql) \
+                    .joins(:translations) \
+                    .order("public_body_translations.name") \
+                    .paginate(:page => params[:page], :per_page => 100)
+            end
+
             respond_to do |format|
                 format.html { render :template => "public_body/list" }
             end
@@ -168,5 +203,18 @@ class PublicBodyController < ApplicationController
         @xapian_requests = perform_search_typeahead(query, PublicBody)
         render :partial => "public_body/search_ahead"
     end
-end
 
+    private
+    def get_public_body_list_translated_condition(table, first_letter=false, locale=nil)
+        result = "(upper(#{table}.name) LIKE upper(?)" \
+            " OR upper(#{table}.notes) LIKE upper (?))"
+        if first_letter
+            result += " AND #{table}.first_letter = ?"
+        end
+        if locale
+            result += " AND #{table}.locale = ?"
+        end
+        result
+    end
+
+end
diff --git a/spec/controllers/public_body_controller_spec.rb b/spec/controllers/public_body_controller_spec.rb
index d4f359a80..70ed482b2 100644
--- a/spec/controllers/public_body_controller_spec.rb
+++ b/spec/controllers/public_body_controller_spec.rb
@@ -1,6 +1,8 @@
 # coding: utf-8
 require File.expand_path(File.dirname(__FILE__) + '/../spec_helper')
 
+require 'nokogiri'
+
 describe PublicBodyController, "when showing a body" do
     render_views
 
@@ -99,6 +101,14 @@ describe PublicBodyController, "when listing bodies" do
         result
     end
 
+    it "with no fallback, should only return bodies from the current locale" do
+        @english_only = make_single_language_example :en
+        @spanish_only = make_single_language_example :es
+        get :list, {:locale => 'es'}
+        assigns[:public_bodies].include?(@english_only).should == false
+        assigns[:public_bodies].include?(@spanish_only).should == true
+    end
+
     it "if fallback is requested, should list all bodies from default locale, even when there are no translations for selected locale" do
         AlaveteliConfiguration.stub!(:public_body_list_fallback_to_default_locale).and_return(true)
         @english_only = make_single_language_example :en
@@ -113,6 +123,14 @@ describe PublicBodyController, "when listing bodies" do
         assigns[:public_bodies].include?(@spanish_only).should == true
     end
 
+    it "if fallback is requested, make sure that there are no duplicates listed" do
+        AlaveteliConfiguration.stub!(:public_body_list_fallback_to_default_locale).and_return(true)
+        get :list, {:locale => 'es'}
+        pb_ids = assigns[:public_bodies].map { |pb| pb.id }
+        unique_pb_ids = pb_ids.uniq
+        pb_ids.sort.should === unique_pb_ids.sort
+    end
+
     it 'should show public body names in the selected locale language if present' do
         get :list, {:locale => 'es'}
         response.should contain('El Department for Humpadinking')
@@ -124,6 +142,18 @@ describe PublicBodyController, "when listing bodies" do
         response.should_not contain('Internal admin authority')
     end
 
+    it 'should order on the translated name, even with the fallback' do
+      # The names of each public body is in:
+      #    <span class="head"><a>Public Body Name</a></span>
+      # ... eo extract all of those, and check that they are ordered:
+      AlaveteliConfiguration.stub!(:public_body_list_fallback_to_default_locale).and_return(true)
+      get :list, {:locale => 'es'}
+      parsed = Nokogiri::HTML(response.body)
+      public_body_names = parsed.xpath '//span[@class="head"]/a/text()'
+      public_body_names = public_body_names.map { |pb| pb.to_s }
+      public_body_names.should == public_body_names.sort
+    end
+
     it 'should show public body names in the selected locale language if present for a locale with underscores' do
         AlaveteliLocalization.set_locales('he_IL en', 'en')
         get :list, {:locale => 'he_IL'}