Merge branch 'release/0.5' into develop

18 files changed, 125 insertions, 39 deletions

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index b0351f7d1..8fd2da54a 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -361,6 +361,27 @@ class ApplicationController < ActionController::Base
     def get_search_page_from_params
         return (params[:page] || "1").to_i
     end
+    def perform_search_typeahead(query, model)
+        # strip out unintended search operators - see
+        # https://github.com/sebbacon/alaveteli/issues/328 
+        # XXX this is a result of the OR hack below -- should fix by
+        # allowing a parameter to perform_search to control the
+        # default operator!
+        query = query.gsub(/(\s-\s|&)/, "")
+        query = query.split(/ +(?!-)/)
+        if query.last.nil? || query.last.strip.length < 3
+            xapian_requests = nil
+        else
+            query = query.join(' OR ')       # XXX: HACK for OR instead of default AND!
+            if model == PublicBody
+                collapse = nil
+            elsif model == InfoRequestEvent
+                collapse = 'request_collapse'
+            end
+            xapian_requests = perform_search([model], query, 'relevant', collapse, 5)
+        end
+        return xapian_requests
+    end
 
     # Store last visited pages, for contact form; but only for logged in users, as otherwise this breaks caching
     def set_last_request(info_request)
diff --git a/app/controllers/public_body_controller.rb b/app/controllers/public_body_controller.rb
index 94d1351db..659433c9e 100644
--- a/app/controllers/public_body_controller.rb
+++ b/app/controllers/public_body_controller.rb
@@ -186,13 +186,7 @@ class PublicBodyController < ApplicationController
         # Since acts_as_xapian doesn't support the Partial match flag, we work around it
         # by making the last work a wildcard, which is quite the same
         query = params[:query]
-        query = query.split(' ')
-        if query.last.nil? || query.last.strip.length < 3
-            @xapian_requests = nil
-        else
-            query = query.join(' OR ')       # XXX: HACK for OR instead of default AND!
-            @xapian_requests = perform_search([PublicBody], query, 'relevant', nil, 5)
-        end
+        @xapian_requests = perform_search_typeahead(query, PublicBody)
         render :partial => "public_body/search_ahead"
     end
 end
diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb
index f3bbd6708..6e33fe043 100644
--- a/app/controllers/request_controller.rb
+++ b/app/controllers/request_controller.rb
@@ -755,13 +755,7 @@ class RequestController < ApplicationController
         # Since acts_as_xapian doesn't support the Partial match flag, we work around it
         # by making the last work a wildcard, which is quite the same
         query = params[:q]
-        query = query.split(' ')
-        if query.last.nil? || query.last.strip.length < 3
-            @xapian_requests = nil
-        else
-            query = query.join(' OR ')       # XXX: HACK for OR instead of default AND!
-            @xapian_requests = perform_search([InfoRequestEvent], query, 'relevant', 'request_collapse', 5)
-        end
+        @xapian_requests = perform_search_typeahead(query, InfoRequestEvent)
         render :partial => "request/search_ahead.rhtml"
     end
 
diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb
index fc29a847c..45b71a3a9 100644
--- a/app/controllers/user_controller.rb
+++ b/app/controllers/user_controller.rb
@@ -116,8 +116,10 @@ class UserController < ApplicationController
             render :action => 'sign'
             return
         else
-            @user_signin = User.authenticate_from_form(params[:user_signin], @post_redirect.reason_params[:user_name] ? true : false)
-            if @user_signin.errors.size > 0
+            if !@post_redirect.nil?
+                @user_signin = User.authenticate_from_form(params[:user_signin], @post_redirect.reason_params[:user_name] ? true : false)
+            end
+            if @post_redirect.nil? || @user_signin.errors.size > 0
                 # Failed to authenticate
                 render :action => 'sign'
                 return
diff --git a/app/helpers/link_to_helper.rb b/app/helpers/link_to_helper.rb
index 5866c31f0..7903dee2a 100755
--- a/app/helpers/link_to_helper.rb
+++ b/app/helpers/link_to_helper.rb
@@ -189,10 +189,14 @@ module LinkToHelper
         url_prefix = "http://" + MySociety::Config.get("DOMAIN", '127.0.0.1:3000')
         url = url_prefix + relative_path
         if !append.nil?
-            env = Rack::MockRequest.env_for(url)
-            req = Rack::Request.new(env)
-            req.path_info += append
-            url = req.url
+            begin
+                env = Rack::MockRequest.env_for(url)
+                req = Rack::Request.new(env)
+                req.path_info += append
+                url = req.url
+            rescue URI::InvalidURIError
+                # don't append to it
+            end
         end
         return url
     end
diff --git a/app/models/foi_attachment.rb b/app/models/foi_attachment.rb
index a14e0b553..dc55ff5f6 100644
--- a/app/models/foi_attachment.rb
+++ b/app/models/foi_attachment.rb
@@ -59,6 +59,7 @@ class FoiAttachment < ActiveRecord::Base
             file.write d
         }
         update_display_size!
+        @cached_body = d
     end
 
     def body
diff --git a/app/views/admin_public_body/_form.rhtml b/app/views/admin_public_body/_form.rhtml
index 1cdc9b3fe..d854b53f5 100644
--- a/app/views/admin_public_body/_form.rhtml
+++ b/app/views/admin_public_body/_form.rhtml
@@ -50,12 +50,13 @@
 <h3>Common Fields</h3>
 
 <p><label for="public_body_tag_string">Tags <small>(space separated; see list of tags on the right; also <strong>not_apply</strong> if FOI and EIR no longer apply to authority, <strong>eir_only</strong> if EIR but not FOI applies to authority, <strong>defunct</strong> if the authority no longer exists; charity:NUMBER if a registered charity)</small></label><br/>
-<%= f.text_field :tag_string, :size => 60  %></p>
+
+<%= text_field :public_body, :tag_string, :size => 60, :id => 'public_body_tag_string'  %></p>
 
 <p><label for="public_body_home_page">Home page <small>(of whole authority, not just their FOI page; set to <strong>blank</strong> (empty string) to guess it from the email)</small></label><br/>
-<%= f.text_field :home_page, :size => 60  %></p>
+<%= text_field :public_body, :home_page, :size => 60, :id => 'public_body_home_page'  %></p>
 
 <p><label for="public_body_last_edit_comment"><strong>Comment</strong> for this edit</label> <small>(put URL or other source of new info)</small><br/>
-<%= f.text_area :last_edit_comment, :rows => 3, :cols => 60  %></p>
+<%= text_area :public_body, :last_edit_comment, :rows => 3, :cols => 60, :id => 'public_body_last_edit_comment'  %></p>
 
 <!--[eoform:public_body]-->
diff --git a/app/views/admin_public_body/edit.rhtml b/app/views/admin_public_body/edit.rhtml
index b91f15a2e..b19477a6b 100644
--- a/app/views/admin_public_body/edit.rhtml
+++ b/app/views/admin_public_body/edit.rhtml
@@ -9,9 +9,9 @@
 <%= render :partial => 'tag_help' %>
 
 <div id="public_body_form">
-    <% form_for @public_body, :url => {:action => 'update'} do |f| %>
-      <%= render :partial => 'form', :locals => {:f => f} %>
-      <p><%= f.submit 'Save', :accesskey => 's' %></p>
+    <% form_tag '../update/' + @public_body.id.to_s do %>
+      <%= render :partial => 'form' %>
+      <p><%= submit_tag 'Save', :accesskey => 's' %></p>
     <% end %>
 
     <p>
diff --git a/app/views/general/exception_caught.rhtml b/app/views/general/exception_caught.rhtml
index b266b53a1..5f0dfe13d 100644
--- a/app/views/general/exception_caught.rhtml
+++ b/app/views/general/exception_caught.rhtml
@@ -19,6 +19,6 @@
 
  <% end %>
   <h2><%= _('Technical details') %></h2>
-  <p><strong><%=@exception_class ? @exception_class : _("Unknown")%></strong></p>
-  <p><strong><%=@exception_message %></strong></p>
+  <p><strong><%= h(@exception_class ? @exception_class : _("Unknown")) %></strong></p>
+  <p><strong><%= h(@exception_message) %></strong></p>
 </div>
diff --git a/config/test.yml b/config/test.yml
index c13b9c9db..693cdd6b8 100644
--- a/config/test.yml
+++ b/config/test.yml
@@ -10,7 +10,7 @@
 SITE_NAME: 'Alaveteli'
 
 # Domain used in URLs generated by scripts (e.g. for going in some emails)
-DOMAIN: 'localhost:3000'
+DOMAIN: 'test.localdomain'
  
 # ISO country code of country currrently deployed in
 # (http://en.wikipedia.org/wiki/ISO_3166-1_alpha-2)
@@ -115,4 +115,8 @@ GAZE_URL: http://gaze.mysociety.org
 # take two arguments: the URL, and a path to an output file.  A static
 # binary of wkhtmltopdf is recommended:
 # http://code.google.com/p/wkhtmltopdf/downloads/list
-HTML_TO_PDF_COMMAND: /usr/local/bin/wkhtmltopdf-amd64
\ No newline at end of file
+HTML_TO_PDF_COMMAND: /usr/local/bin/wkhtmltopdf-amd64
+
+# Exception notifications
+EXCEPTION_NOTIFICATIONS_FROM: do-not-reply-to-this-address@example.com
+EXCEPTION_NOTIFICATIONS_TO:
diff --git a/db/migrate/109_change_sent_at_to_datetime.rb b/db/migrate/109_change_sent_at_to_datetime.rb
new file mode 100644
index 000000000..5fc9b29ae
--- /dev/null
+++ b/db/migrate/109_change_sent_at_to_datetime.rb
@@ -0,0 +1,12 @@
+class ChangeSentAtToDatetime < ActiveRecord::Migration
+    def self.up
+        remove_column :incoming_messages, :sent_at
+        add_column :incoming_messages, :sent_at, :timestamp
+        ActiveRecord::Base.connection.execute("update incoming_messages set last_parsed = null")
+    end
+
+    def self.down
+        remove_column :incoming_messages, :sent_at
+        add_column :incoming_messages, :sent_at, :time
+    end
+end
diff --git a/public/robots.txt b/public/robots.txt
index 029ae0dbf..6a8628c93 100644
--- a/public/robots.txt
+++ b/public/robots.txt
@@ -24,3 +24,9 @@ Disallow: /feed/
 Disallow: /profile/
 Disallow: /signin
 Disallow: /body/*/view_email$
+
+# The following adding Jan 2012 to stop robots crawling pages
+# generated in error (see
+# https://github.com/sebbacon/alaveteli/issues/311).  Can be removed
+# later in 2012 when the error pages have been dropped from the index
+Disallow: *.json.j*
diff --git a/spec/controllers/public_body_controller_spec.rb b/spec/controllers/public_body_controller_spec.rb
index 8182e1331..3996bd520 100644
--- a/spec/controllers/public_body_controller_spec.rb
+++ b/spec/controllers/public_body_controller_spec.rb
@@ -175,20 +175,20 @@ describe PublicBodyController, "when doing type ahead searches" do
     fixtures :public_bodies, :public_body_translations, :public_body_versions, :users, :info_requests, :raw_emails, :incoming_messages, :outgoing_messages, :comments, :info_request_events, :track_things
 
     it "should return nothing for the empty query string" do
-        get :search_typeahead, :q => ""
+        get :search_typeahead, :query => ""
         response.should render_template('public_body/_search_ahead')
         assigns[:xapian_requests].should be_nil
     end
     
     it "should return a body matching the given keyword, but not users with a matching description" do
-        get :search_typeahead, :q => "Geraldine"
+        get :search_typeahead, :query => "Geraldine"
         response.should render_template('public_body/_search_ahead')
         assigns[:xapian_requests].results.size.should == 1
         assigns[:xapian_requests].results[0][:model].name.should == public_bodies(:geraldine_public_body).name
     end
 
     it "should return all requests matching any of the given keywords" do
-        get :search_typeahead, :q => "Geraldine Humpadinking"
+        get :search_typeahead, :query => "Geraldine Humpadinking"
         response.should render_template('public_body/_search_ahead')
         assigns[:xapian_requests].results.size.should == 2
         assigns[:xapian_requests].results[0][:model].name.should == public_bodies(:humpadink_public_body).name
@@ -196,14 +196,14 @@ describe PublicBodyController, "when doing type ahead searches" do
     end
 
     it "should return requests matching the given keywords in any of their locales" do
-        get :search_typeahead, :q => "baguette" # part of the spanish notes
+        get :search_typeahead, :query => "baguette" # part of the spanish notes
         response.should render_template('public_body/_search_ahead')
         assigns[:xapian_requests].results.size.should == 1
         assigns[:xapian_requests].results[0][:model].name.should == public_bodies(:humpadink_public_body).name
     end
 
     it "should not return  matches for short words" do
-        get :search_typeahead, :q => "b" 
+        get :search_typeahead, :query => "b" 
         response.should render_template('public_body/_search_ahead')
         assigns[:xapian_requests].should be_nil
     end
diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb
index 2bf45e914..fc62edc14 100644
--- a/spec/controllers/request_controller_spec.rb
+++ b/spec/controllers/request_controller_spec.rb
@@ -1487,11 +1487,27 @@ describe RequestController, "when doing type ahead searches" do
         assigns[:xapian_requests].results[1][:model].title.should == info_requests(:naughty_chicken_request).title
     end
 
-    it "should not return  matches for short words" do
+    it "should not return matches for short words" do
         get :search_typeahead, :q => "a" 
         response.should render_template('request/_search_ahead.rhtml')
         assigns[:xapian_requests].should be_nil
     end
+
+    it "should not give an error when user users unintended search operators" do
+        for phrase in ["Marketing/PR activities - Aldborough E-Act Free Schoo",
+                       "Request for communications between DCMS/Ed Vaizey and ICO from Jan 1st 2011 - May ",
+                       "Bellevue Road Ryde Isle of Wight PO33 2AR - what is the"]
+            lambda {
+                get :search_typeahead, :q => phrase
+            }.should_not raise_error(StandardError)
+        end
+    end
+
+    it "should return all requests matching any of the given keywords" do
+        get :search_typeahead, :q => "dog -chicken"
+        assigns[:xapian_requests].results.size.should == 1
+    end
+
 end
 
 
diff --git a/spec/controllers/user_controller_spec.rb b/spec/controllers/user_controller_spec.rb
index c13d7c9fc..2560b48c7 100644
--- a/spec/controllers/user_controller_spec.rb
+++ b/spec/controllers/user_controller_spec.rb
@@ -109,6 +109,19 @@ describe UserController, "when signing in" do
         response.should_not send_email
     end
 
+    it "should not log you in if you use an invalid PostRedirect token, and shouldn't give 500 error either" do
+        ActionController::Routing::Routes.filters.clear
+        get :signin, :r => "/list"
+        response.should render_template('sign')
+        post_redirect = "something invalid"
+        lambda {
+            post :signin, { :user_signin => { :email => 'bob@localhost', :password => 'jonespassword' },
+                :token => post_redirect
+            }
+        }.should_not raise_error(NoMethodError)
+        response.should render_template('sign')
+    end
+
 # No idea how to test this in the test framework :(
 #    it "should have set a long lived cookie if they picked remember me, session cookie if they didn't" do
 #        get :signin, :r => "/list"
diff --git a/spec/helpers/link_to_helper_spec.rb b/spec/helpers/link_to_helper_spec.rb
index aae00c298..f85d2e70d 100644
--- a/spec/helpers/link_to_helper_spec.rb
+++ b/spec/helpers/link_to_helper_spec.rb
@@ -20,5 +20,17 @@ describe LinkToHelper do
         end
         
     end
+
+    describe "when appending something to a URL" do
+        it 'should append to things without query strings' do
+            main_url('/a', '.json').should == 'http://test.localdomain/a.json'
+        end
+        it 'should append to things with query strings' do
+            main_url('/a?z=1', '.json').should == 'http://test.localdomain/a.json?z=1'
+        end
+        it 'should fail silently with invalid URLs' do
+            main_url('/a?z=9%', '.json').should == 'http://test.localdomain/a?z=9%'
+        end
+    end
     
 end
diff --git a/spec/models/incoming_message_spec.rb b/spec/models/incoming_message_spec.rb
index f514d6546..7808ef24c 100644
--- a/spec/models/incoming_message_spec.rb
+++ b/spec/models/incoming_message_spec.rb
@@ -14,6 +14,8 @@ describe IncomingMessage, " when dealing with incoming mail" do
     end
 
     it "should return the mail Date header date for sent at" do
+        @im.parse_raw_email!(true)
+        @im.reload
         @im.sent_at.should == @im.mail.date
     end
 
diff --git a/vendor/plugins/acts_as_xapian/lib/acts_as_xapian.rb b/vendor/plugins/acts_as_xapian/lib/acts_as_xapian.rb
index 39cfe929f..70605ad04 100644
--- a/vendor/plugins/acts_as_xapian/lib/acts_as_xapian.rb
+++ b/vendor/plugins/acts_as_xapian/lib/acts_as_xapian.rb
@@ -584,8 +584,8 @@ module ActsAsXapian
     end
     
     def ActsAsXapian._is_xapian_db(path)
-        exists = File.exist?(File.join(path, "iamflint")) or File.exist?(File.join(path, "iamchert"))
-        return exists
+        is_db = File.exist?(File.join(path, "iamflint")) || File.exist?(File.join(path, "iamchert"))
+        return is_db
     end
     
     # You must specify *all* the models here, this totally rebuilds the Xapian
@@ -633,6 +633,7 @@ module ActsAsXapian
         # Rename into place
         temp_path = old_path + ".tmp"
         if File.exist?(temp_path)
+            @@db_path = old_path
             raise "temporary database found " + temp_path + " which is not Xapian flint database, please delete for me" if not ActsAsXapian._is_xapian_db(temp_path)
             FileUtils.rm_r(temp_path)
         end
@@ -643,7 +644,10 @@ module ActsAsXapian
 
         # Delete old database
         if File.exist?(temp_path)
-            raise "old database now at " + temp_path + " is not Xapian flint database, please delete for me" if not ActsAsXapian._is_xapian_db(temp_path)
+            if not ActsAsXapian._is_xapian_db(temp_path)
+                @@db_path = old_path
+                raise "old database now at " + temp_path + " is not Xapian flint database, please delete for me"
+            end
             FileUtils.rm_r(temp_path)
         end