Use names in URLs of requests.

15 files changed, 85 insertions, 47 deletions

diff --git a/app/controllers/body_controller.rb b/app/controllers/body_controller.rb
index 6ad713455..103b500b8 100644
--- a/app/controllers/body_controller.rb
+++ b/app/controllers/body_controller.rb
@@ -4,11 +4,16 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: body_controller.rb,v 1.6 2008-02-27 12:09:03 francis Exp $
+# $Id: body_controller.rb,v 1.7 2008-02-27 13:59:51 francis Exp $
 
 class BodyController < ApplicationController
     # XXX tidy this up with better error messages, and a more standard infrastructure for the redirect to canonical URL
     def show
+        if MySociety::Format.simplify_url_part(params[:url_name]) != params[:url_name]
+            redirect_to :url_name =>  MySociety::Format.simplify_url_part(params[:url_name])
+            return
+        end
+
         @public_bodies = PublicBody.find(:all,
             :conditions => [ "url_name = ?", params[:url_name] ])
         if @public_bodies.size > 1
diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb
index 3d7e1c869..d02694183 100644
--- a/app/controllers/request_controller.rb
+++ b/app/controllers/request_controller.rb
@@ -4,12 +4,22 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: request_controller.rb,v 1.59 2008-02-25 11:17:29 francis Exp $
+# $Id: request_controller.rb,v 1.60 2008-02-27 13:59:51 francis Exp $
 
 class RequestController < ApplicationController
     
     def show
-        @info_request = InfoRequest.find(params[:id])
+        # Look up by old style numeric identifiers
+        if params[:url_title].match(/^[0-9]+$/)
+            @info_request = InfoRequest.find(params[:url_title].to_i)
+            redirect_to request_url(@info_request)
+            return
+        end
+    
+        # Look up by new style text names 
+        @info_request = InfoRequest.find_by_url_title(params[:url_title])
+        
+        # Other parameters
         @info_request_events = @info_request.info_request_events
         @info_request_events.sort! { |a,b| a.created_at <=> b.created_at } 
         @status = @info_request.calculate_status
@@ -110,7 +120,7 @@ class RequestController < ApplicationController
             # XXX send_message needs the database id, so we send after saving, which isn't ideal if the request broke here.
             @outgoing_message.send_message
             flash[:notice] = "Your Freedom of Information request has been created and sent on its way."
-            redirect_to show_request_url(:id => @info_request)
+            redirect_to show_request_url(:url_title => @info_request.url_title)
         else
             # do nothing - as "authenticated?" has done the redirect to signin page for us
         end
@@ -125,7 +135,7 @@ class RequestController < ApplicationController
             if !params[:submitted_describe_state].nil?
                 flash[:notice] = "The status of this request was made up to date elsewhere while you were filling in the form."
             end
-            redirect_to show_request_url(:id => @info_request)
+            redirect_to show_request_url(:url_title => @info_request.url_title)
             return
         end
 
@@ -137,7 +147,7 @@ class RequestController < ApplicationController
 
         if @last_info_request_event_id.nil?
             flash[:notice] = "Internal error - awaiting description, but no event to describe"
-            redirect_to show_request_url(:id => @info_request)
+            redirect_to show_request_url(:url_title => @info_request.url_title)
             return
         end
 
@@ -169,10 +179,10 @@ class RequestController < ApplicationController
             # Display appropriate next page (e.g. help for complaint etc.)
             if @info_request.calculate_status == 'waiting_response'
                 flash[:notice] = "<p>Thank you! Hopefully your wait isn't too long.</p> <p>By law, you should get a response before the end of <strong>" + simple_date(@info_request.date_response_required_by) + "</strong>.</p>"
-                redirect_to show_request_url(:id => @info_request)
+                redirect_to show_request_url(:url_title => @info_request.url_title)
             elsif @info_request.calculate_status == 'waiting_response_overdue'
                 flash[:notice] = "<p>Thank you! Hope you don't have to wait much longer.</p> <p>By law, you should have got a response before the end of <strong>" + simple_date(@info_request.date_response_required_by) + "</strong>.</p>"
-                redirect_to show_request_url(:id => @info_request)
+                redirect_to show_request_url(:url_title => @info_request.url_title)
             elsif @info_request.calculate_status == 'rejected'
                 # XXX explain how to complain
                 flash[:notice] = "Oh no! Sorry to hear that your request was rejected. Here is what to do now."
@@ -180,17 +190,17 @@ class RequestController < ApplicationController
             elsif @info_request.calculate_status == 'successful'
                 flash[:notice] = "We're glad you got all the information that you wanted. Thank you for using foi.mysociety.org"
                 # XXX quiz them here for a comment
-                redirect_to show_request_url(:id => @info_request)
+                redirect_to show_request_url(:url_title => @info_request.url_title)
             elsif @info_request.calculate_status == 'partially_successful'
                 flash[:notice] = "We're glad you got some of the information that you wanted."
                 # XXX explain how to complain / quiz them for a comment
-                redirect_to show_request_url(:id => @info_request)
+                redirect_to show_request_url(:url_title => @info_request.url_title)
             elsif @info_request.calculate_status == 'waiting_clarification'
                 flash[:notice] = "Please write your follow up message containing the necessary clarifications below."
                 redirect_to show_response_url(:id => @info_request.id, :incoming_message_id => @events_needing_description[-1].params[:incoming_message_id])
             elsif @info_request.calculate_status == 'requires_admin'
                 flash[:notice] = "Thanks! The foi.mysociety.org team have been notified."
-                redirect_to show_request_url(:id => @info_request)
+                redirect_to show_request_url(:url_title => @info_request.url_title)
             else
                 raise "unknown calculate_status " + @info_request.calculate_status
             end
@@ -242,7 +252,7 @@ class RequestController < ApplicationController
                 @outgoing_message.send_message
                 @outgoing_message.save!
                 flash[:notice] = "Your follow up message has been created and sent on its way."
-                redirect_to show_request_url(:id => @info_request)
+                redirect_to show_request_url(:url_title => @info_request.url_title)
             else
                 # do nothing - as "authenticated?" has done the redirect to signin page for us
             end
diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb
index 50f8ece10..94fe731ba 100644
--- a/app/controllers/user_controller.rb
+++ b/app/controllers/user_controller.rb
@@ -4,13 +4,14 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: user_controller.rb,v 1.29 2008-02-27 12:20:22 francis Exp $
+# $Id: user_controller.rb,v 1.30 2008-02-27 13:59:52 francis Exp $
 
 class UserController < ApplicationController
     # Show page about a set of users with same url name
     def show
-        if  MySociety::Format.simplify_url_part(params[:url_name]) != params[:url_name]
+        if MySociety::Format.simplify_url_part(params[:url_name]) != params[:url_name]
             redirect_to :url_name =>  MySociety::Format.simplify_url_part(params[:url_name])
+            return
         end
 
         @display_users = User.find(:all, :conditions => [ "url_name = ?", params[:url_name] ], :order => "created_at desc")
diff --git a/app/helpers/link_to_helper.rb b/app/helpers/link_to_helper.rb
index 8d4a1baca..6d57ed353 100644
--- a/app/helpers/link_to_helper.rb
+++ b/app/helpers/link_to_helper.rb
@@ -5,7 +5,7 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: link_to_helper.rb,v 1.18 2008-02-27 12:18:28 francis Exp $
+# $Id: link_to_helper.rb,v 1.19 2008-02-27 13:59:52 francis Exp $
 
 module LinkToHelper
 
@@ -13,7 +13,7 @@ module LinkToHelper
    
     # Requests
     def request_url(info_request)
-        return show_request_url(:id => info_request, :only_path => true)
+        return show_request_url(:url_title => info_request.url_title, :only_path => true)
     end
     def request_link(info_request)
         link_to h(info_request.title), request_url(info_request)
@@ -74,11 +74,7 @@ module LinkToHelper
     end
 
 
-    def info_request_link(info_request)
-        link_to h(info_request.title), show_request_url(:id => info_request)
-    end
-
-
+    # Admin pages
     def admin_url(relative_path)
         admin_url_prefix = MySociety::Config.get("ADMIN_BASE_URL", "/admin/")
         return admin_url_prefix + relative_path
diff --git a/app/models/info_request.rb b/app/models/info_request.rb
index 72bbf8b6a..c6dc88aea 100644
--- a/app/models/info_request.rb
+++ b/app/models/info_request.rb
@@ -20,12 +20,13 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: info_request.rb,v 1.47 2008-02-27 12:04:10 francis Exp $
+# $Id: info_request.rb,v 1.48 2008-02-27 13:59:52 francis Exp $
 
 require 'digest/sha1'
 
 class InfoRequest < ActiveRecord::Base
     validates_presence_of :title, :message => "^Please enter a summary of your request"
+    validates_format_of :title, :with => /[a-z]/, :message => "^Please write a summary with some text in it", :if => Proc.new { |info_request| !info_request.title.nil? && !info_request.title.empty? }
 
     belongs_to :user
     #validates_presence_of :user_id # breaks during construction of new ones :(
@@ -62,6 +63,23 @@ class InfoRequest < ActiveRecord::Base
     end
 
 public
+    # When name is changed, also change the url name
+    def title=(title)
+        write_attribute(:title, title)
+        self.update_url_title
+    end
+    def update_url_title
+        url_title = MySociety::Format.simplify_url_part(self.title)
+        if url_title.size > 32
+            url_title = url_title[0..31]
+        end
+        # For request with same name as others, tag on the request numeric id
+        while not InfoRequest.find_by_url_title(url_title).nil?
+            url_title += "-" + self.id.to_s
+        end
+        write_attribute(:url_title, url_title)
+    end
+
     # Email which public body should use to respond to request. This is in
     # the format PREFIXrequest-ID-HASH@DOMAIN. Here ID is the id of the 
     # FOI request, and HASH is a signature for that id.
diff --git a/app/models/request_mailer.rb b/app/models/request_mailer.rb
index 20ac78b70..d4f85659b 100644
--- a/app/models/request_mailer.rb
+++ b/app/models/request_mailer.rb
@@ -4,7 +4,7 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: request_mailer.rb,v 1.23 2008-02-22 01:58:36 francis Exp $
+# $Id: request_mailer.rb,v 1.24 2008-02-27 13:59:52 francis Exp $
 
 class RequestMailer < ApplicationMailer
     
@@ -55,7 +55,7 @@ class RequestMailer < ApplicationMailer
         @recipients = @from
         @subject = "Unusual FOI response, requires admin attention"
         # XXX these are repeats of things in helpers/link_to_helper.rb, and shouldn't be
-        url =  show_request_url(:id => info_request)
+        url =  show_request_url(:url_title => info_request.url_title)
         admin_url =  MySociety::Config.get("ADMIN_BASE_URL", "/admin/") + 'request/show/' + info_request.id.to_s
         @body       = {:info_request => info_request, :url => url, :admin_url => admin_url }
     end
diff --git a/app/views/admin_request/list.rhtml b/app/views/admin_request/list.rhtml
index 9472ac8c1..0ffd07d0e 100644
--- a/app/views/admin_request/list.rhtml
+++ b/app/views/admin_request/list.rhtml
@@ -4,15 +4,15 @@
 
 <table>
   <tr>
-  <% for column in InfoRequest.content_columns %>
-    <th><%= column.human_name %></th>
+  <% for column in InfoRequest.content_columns.map { |c| c.human_name } - [ "Url title" ] %>
+    <th><%= column %></th>
   <% end %>
   </tr>
   
 <% for info_request in @info_requests %>
   <tr class="<%= cycle('odd', 'even') %>">
     <td><%= link_to h(info_request.title), 'show/' + info_request.id.to_s %></td>
-  <% for column in InfoRequest.content_columns.map { |c| c.name } - [ "title" ] %>
+  <% for column in InfoRequest.content_columns.map { |c| c.name } - [ "title", "url_title" ] %>
     <td><%=h info_request.send(column) %></td>
   <% end %>
   </tr>
diff --git a/config/routes.rb b/config/routes.rb
index cc5032f09..17124bed2 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -4,7 +4,7 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: routes.rb,v 1.39 2008-02-27 12:18:29 francis Exp $
+# $Id: routes.rb,v 1.40 2008-02-27 13:59:52 francis Exp $
 
 ActionController::Routing::Routes.draw do |map|
     # The priority is based upon order of creation: first created -> highest priority.
@@ -20,7 +20,7 @@ ActionController::Routing::Routes.draw do |map|
       request.request_list   '/list',        :action => 'list'
       request.new_request    '/new',         :action => 'new'
       request.new_request_to_body    '/new/:public_body_id',         :action => 'new'
-      request.show_request     '/request/:id', :action => 'show'
+      request.show_request     '/request/:url_title', :action => 'show'
       request.describe_state   '/request/:id/describe', :action => 'describe_state'
       request.show_response_no_followup    '/request/:id/response', :action => 'show_response'
       request.show_response    '/request/:id/response/:incoming_message_id', :action => 'show_response'
diff --git a/db/migrate/037_add_url_name.rb b/db/migrate/037_add_url_name.rb
index dabb4889f..0a667e0d3 100644
--- a/db/migrate/037_add_url_name.rb
+++ b/db/migrate/037_add_url_name.rb
@@ -13,6 +13,6 @@ class AddUrlName < ActiveRecord::Migration
 
     def self.down
         remove_column :public_bodies, :url_name
-        remove_column :public_body_versions, :url_name, :text
+        remove_column :public_body_versions, :url_name
     end
 end
diff --git a/db/schema.rb b/db/schema.rb
index 08ecdcce6..0e8c26f80 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -9,7 +9,7 @@
 #
 # It's strongly recommended to check this file into your version control system.
 
-ActiveRecord::Schema.define(:version => 38) do
+ActiveRecord::Schema.define(:version => 39) do
 
   create_table "incoming_messages", :force => true do |t|
     t.integer  "info_request_id",                    :null => false
@@ -36,10 +36,12 @@ ActiveRecord::Schema.define(:version => 38) do
     t.string   "described_state",                            :null => false
     t.boolean  "awaiting_description", :default => false,    :null => false
     t.string   "prominence",           :default => "normal", :null => false
+    t.text     "url_title",                                  :null => false
   end
 
   add_index "info_requests", ["created_at"], :name => "index_info_requests_on_created_at"
   add_index "info_requests", ["title"], :name => "index_info_requests_on_title"
+  add_index "info_requests", ["url_title"], :name => "index_info_requests_on_url_title", :unique => true
 
   create_table "outgoing_messages", :force => true do |t|
     t.integer  "info_request_id",              :null => false
diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb
index 22f354cae..4d68c2b8c 100644
--- a/spec/controllers/request_controller_spec.rb
+++ b/spec/controllers/request_controller_spec.rb
@@ -53,28 +53,28 @@ describe RequestController, "when showing one request" do
     fixtures :info_requests, :info_request_events, :public_bodies, :users, :incoming_messages, :outgoing_messages # all needed as integrating views
   
     it "should be successful" do
-        get :show, :id => 101
+        get :show, :url_title => 'why_do_you_have_such_a_fancy_dog'
         response.should be_success
     end
 
     it "should render with 'show' template" do
-        get :show, :id => 101
+        get :show, :url_title => 'why_do_you_have_such_a_fancy_dog'
         response.should render_template('show')
     end
 
     it "should assign the request" do
-        get :show, :id => 101
+        get :show, :url_title => 'why_do_you_have_such_a_fancy_dog'
         assigns[:info_request].should == info_requests(:fancy_dog_request)
     end
 
     it "should show incoming messages" do
-        get :show, :id => 101
+        get :show, :url_title => 'why_do_you_have_such_a_fancy_dog'
         size_before = assigns[:info_request_events].size
 
         ir = info_requests(:fancy_dog_request) 
         receive_incoming_mail('incoming-request-plain.email', ir.incoming_email)
 
-        get :show, :id => 101
+        get :show, :url_title => 'why_do_you_have_such_a_fancy_dog'
         (assigns[:info_request_events].size - size_before).should == 1
     end
 end
@@ -141,7 +141,7 @@ describe RequestController, "when creating a new request" do
         mail = deliveries[0]
         mail.body.should =~ /This is a silly letter. It is too short to be interesting./
 
-        response.should redirect_to(:controller => 'request', :action => 'show', :id => ir.id)
+        response.should redirect_to(:controller => 'request', :action => 'show', :url_title => ir.url_title)
     end
 
     it "should give an error if the same request is submitted twice" do
@@ -227,7 +227,7 @@ describe RequestController, "when sending a followup message" do
         mail.body.should =~ /What a useless response! You suck./
         mail.to_addrs.to_s.should == "FOI Person <foiperson@localhost>"
 
-        response.should redirect_to(:controller => 'request', :action => 'show', :id => info_requests(:fancy_dog_request))
+        response.should redirect_to(:controller => 'request', :action => 'show', :url_title => info_requests(:fancy_dog_request).url_title)
     end
 
 
diff --git a/spec/controllers/user_controller_spec.rb b/spec/controllers/user_controller_spec.rb
index 331428cf8..3bf7aaf51 100644
--- a/spec/controllers/user_controller_spec.rb
+++ b/spec/controllers/user_controller_spec.rb
@@ -5,27 +5,27 @@ describe UserController, "when showing a user" do
     fixtures :users, :outgoing_messages, :incoming_messages, :info_requests, :info_request_events
   
     it "should be successful" do
-        get :show, :url_name => "bob-smith"
+        get :show, :url_name => "bob_smith"
         response.should be_success
     end
 
     it "should redirect to lower case name if given one with capital letters" do
-        get :show, :url_name => "Bob-Smith"
-        response.should redirect_to(:controller => 'user', :action => 'show', :url_name => "bob-smith")
+        get :show, :url_name => "Bob_Smith"
+        response.should redirect_to(:controller => 'user', :action => 'show', :url_name => "bob_smith")
     end
 
     it "should render with 'show' template" do
-        get :show, :url_name => "bob-smith"
+        get :show, :url_name => "bob_smith"
         response.should render_template('show')
     end
 
     it "should assign the user" do
-        get :show, :url_name => "bob-smith"
+        get :show, :url_name => "bob_smith"
         assigns[:display_users].should == [ users(:bob_smith_user) ]
     end
     
     it "should assign the user for a more complex name" do
-        get :show, :url_name => "silly-emnameem"
+        get :show, :url_name => "silly_emnameem"
         assigns[:display_users].should == [ users(:silly_name_user) ]
     end
 
diff --git a/spec/fixtures/info_requests.yml b/spec/fixtures/info_requests.yml
index d9aa0d627..a849542b8 100644
--- a/spec/fixtures/info_requests.yml
+++ b/spec/fixtures/info_requests.yml
@@ -1,6 +1,7 @@
 fancy_dog_request:
     id: 101
     title: Why do you have such a fancy dog?
+    url_title: why_do_you_have_such_a_fancy_dog
     created_at: 2007-10-11 18:15:57
     updated_at: 2007-10-11 18:15:57
     public_body_id: 2
@@ -10,6 +11,7 @@ fancy_dog_request:
 naughty_chicken_request:
     id: 103
     title: How much public money is wasted on breeding naughty chickens?
+    url_title: how_much_public_money_is_wasted_o
     created_at: 2007-10-13 18:15:57
     updated_at: 2007-10-13 18:15:57
     public_body_id: 2
diff --git a/spec/fixtures/users.yml b/spec/fixtures/users.yml
index 9daa34bd0..f7ae3ba62 100644
--- a/spec/fixtures/users.yml
+++ b/spec/fixtures/users.yml
@@ -1,7 +1,7 @@
 bob_smith_user: 
   id: "1"
   name: Bob Smith
-  url_name: bob-smith
+  url_name: bob_smith
   email: bob@localhost
   salt: "-6116981980.392287733335677"
   hashed_password: 6b7cd45a5f35fd83febc0452a799530398bfb6e8 # jonespassword
@@ -11,7 +11,7 @@ bob_smith_user:
 silly_name_user: 
   id: "2"
   name: "Silly <em>Name</em>"
-  url_name: silly-emnameem
+  url_name: silly_emnameem
   email: silly@localhost
   salt: "-6116981980.392287733335677"
   hashed_password: 6b7cd45a5f35fd83febc0452a799530398bfb6e8 # jonespassword
diff --git a/todo.txt b/todo.txt
index 1ec42df8a..2b85a0516 100644
--- a/todo.txt
+++ b/todo.txt
@@ -1,5 +1,7 @@
-and request!
+This is knackered:
+http://foi.mysociety.org/request/14/response/44/attach/3/Marie's%20letter.tif
 
+Load errors
 
 FOI requests to use to test it
 ==============================
@@ -16,6 +18,8 @@ BAILII - relationship with law courts, robots.txt ?
 Next
 ====
 
+Update test code
+
 Short name for highways agency
 
 Search and replace text "FOI" and "Freedom of Information" out the way more