Store URL name in database.

Allow blank short names.

16 files changed, 98 insertions, 40 deletions

diff --git a/app/controllers/application.rb b/app/controllers/application.rb
index 83c606ad2..f518a89e9 100644
--- a/app/controllers/application.rb
+++ b/app/controllers/application.rb
@@ -6,7 +6,7 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: application.rb,v 1.28 2008-01-29 03:05:46 francis Exp $
+# $Id: application.rb,v 1.29 2008-02-27 12:04:10 francis Exp $
 
 
 class ApplicationController < ActionController::Base
@@ -44,7 +44,7 @@ class ApplicationController < ActionController::Base
 
     def authenticated_as_user?(user, reason_params) 
         reason_params[:user_name] = user.name
-        reason_params[:user_url] = show_user_url(:simple_name => simplify_url_part(user.name))
+        reason_params[:user_url] = show_user_url(:simple_name =>  MySociety::Format.simplify_url_part(user.name))
         if session[:user_id]
             if session[:user_id] == user.id
                 # They are logged in as the right user
diff --git a/app/controllers/body_controller.rb b/app/controllers/body_controller.rb
index 7f2d1fe05..a557c0cfc 100644
--- a/app/controllers/body_controller.rb
+++ b/app/controllers/body_controller.rb
@@ -4,27 +4,27 @@
 # 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.4 2008-02-22 13:49:37 francis Exp $
+# $Id: body_controller.rb,v 1.5 2008-02-27 12:04:10 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
         @public_bodies = PublicBody.find(:all,
-            :conditions => [ "regexp_replace(replace(lower(short_name), ' ', '-'), '[^a-z0-9_-]', '', 'g') = ?", params[:simple_short_name] ])
+            :conditions => [ "url_name = ?", params[:simple_short_name] ])
         if @public_bodies.size > 1
-            raise "Two bodies with the same simplified short name: " . params[:simple_short_name]
+            raise "Two bodies with the same URL name: " . params[:simple_short_name]
         end
         # If none found, then search the history of short names, and do a redirect
         if @public_bodies.size == 0
             @public_bodies = PublicBody.find(:all,
-                :conditions => [ "id in (select public_body_id from public_body_versions where regexp_replace(replace(lower(short_name), ' ', '-'), '[^a-z0-9_-]', '', 'g') = ?)", params[:simple_short_name] ])
+                :conditions => [ "id in (select public_body_id from public_body_versions where url_name = ?)", params[:simple_short_name] ])
             if @public_bodies.size > 1
-                raise "Two bodies with the same historical simplified short name: " . params[:simple_short_name]
+                raise "Two bodies with the same historical URL name: " . params[:simple_short_name]
             end
             if @public_bodies.size == 0
                 raise "None found" # XXX proper 404
             end
-            redirect_to show_public_body_url(:simple_short_name => simplify_url_part(@public_bodies[0].short_name))
+            redirect_to show_public_body_url(:simple_short_name => @public_bodies[0].url_name)
         end
         @public_body = @public_bodies[0]
     end
diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb
index baeffc7db..8dc056167 100644
--- a/app/controllers/user_controller.rb
+++ b/app/controllers/user_controller.rb
@@ -4,13 +4,13 @@
 # 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.26 2008-02-20 12:51:29 francis Exp $
+# $Id: user_controller.rb,v 1.27 2008-02-27 12:04:10 francis Exp $
 
 class UserController < ApplicationController
     # XXX See controllers/application.rb simplify_url_part for reverse of expression in SQL below
     def show
-        if simplify_url_part(params[:simple_name]) != params[:simple_name]
-            redirect_to :simple_name => simplify_url_part(params[:simple_name])
+        if  MySociety::Format.simplify_url_part(params[:simple_name]) != params[:simple_name]
+            redirect_to :simple_name =>  MySociety::Format.simplify_url_part(params[:simple_name])
         end
 
         @display_users = User.find(:all, :conditions => [ "regexp_replace(replace(lower(name), ' ', '-'), '[^a-z0-9_-]', '', 'g') = ?", params[:simple_name] ], :order => "created_at desc")
diff --git a/app/helpers/link_to_helper.rb b/app/helpers/link_to_helper.rb
index 0f003f913..9fb26a4f1 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.15 2008-02-22 02:10:14 francis Exp $
+# $Id: link_to_helper.rb,v 1.16 2008-02-27 12:04:10 francis Exp $
 
 module LinkToHelper
 
@@ -27,10 +27,10 @@ module LinkToHelper
  
     # Public bodies
     def public_body_url(public_body)
-        return show_public_body_url(:simple_short_name => simplify_url_part(public_body.short_name), :only_path => true)
+        return show_public_body_url(:simple_short_name => public_body.url_name, :only_path => true)
     end
     def public_body_link_short(public_body)
-        link_to h(public_body.short_name), public_body_url(public_body)
+        link_to h(public_body.short_or_long_name), public_body_url(public_body)
     end
     def public_body_link(public_body)
         link_to h(public_body.name), public_body_url(public_body)
@@ -42,12 +42,12 @@ module LinkToHelper
         link_to h(public_body.name), public_body_admin_url(public_body)
     end
     def public_body_admin_link_short(public_body)
-        link_to h(public_body.short_name), public_body_admin_url(public_body)
+        link_to h(public_body.short_or_long_name), public_body_admin_url(public_body)
     end
 
     # Users
     def user_url(user)
-        return show_user_url(:simple_name => simplify_url_part(user.name), :only_path => true)
+        return show_user_url(:simple_name => MySociety::Format.simplify_url_part(user.name), :only_path => true)
     end
     def user_link(user)
         link_to h(user.name), user_url(user)
@@ -79,16 +79,6 @@ module LinkToHelper
     end
 
 
-    # Simplified links to our objects
-    # XXX See controllers/user_controller.rb controllers/body_controller.rb for inverse
-    # XXX consolidate somehow with stuff in helpers/application_helper.rb
-    def simplify_url_part(text)
-        text = text.downcase # this also clones the string, if we use downcase! we modify the original
-        text.gsub!(/ /, "-")
-        text.gsub!(/[^a-z0-9_-]/, "")
-        text
-    end
-
     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/incoming_message.rb b/app/models/incoming_message.rb
index 7908f21e7..334c8f2ed 100644
--- a/app/models/incoming_message.rb
+++ b/app/models/incoming_message.rb
@@ -18,7 +18,7 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: incoming_message.rb,v 1.45 2008-02-26 15:13:51 francis Exp $
+# $Id: incoming_message.rb,v 1.46 2008-02-27 12:04:10 francis Exp $
 
 
 # TODO
@@ -100,10 +100,10 @@ class IncomingMessage < ActiveRecord::Base
         # if they are public anyway.  For now just be precautionary and only
         # put in descriptions of them in square brackets.
         if not self.info_request.public_body.request_email.empty?
-            text = text.gsub(self.info_request.public_body.request_email, "[" + self.info_request.public_body.short_name + " request email]")
+            text = text.gsub(self.info_request.public_body.request_email, "[" + self.info_request.public_body.short_or_long_name + " request email]")
         end
         if not self.info_request.public_body.complaint_email.empty?
-            text = text.gsub(self.info_request.public_body.complaint_email, "[" + self.info_request.public_body.short_name + " complaint email]")
+            text = text.gsub(self.info_request.public_body.complaint_email, "[" + self.info_request.public_body.short_or_long_name + " complaint email]")
         end
         text = text.gsub(self.info_request.incoming_email, "[FOI #" + self.info_request.id.to_s + " email]")
         text = text.gsub(self.info_request.envelope_email, "[FOI #" + self.info_request.id.to_s + " bounce email]")
diff --git a/app/models/info_request.rb b/app/models/info_request.rb
index 3ea3b59cc..72bbf8b6a 100644
--- a/app/models/info_request.rb
+++ b/app/models/info_request.rb
@@ -20,7 +20,7 @@
 # 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.46 2008-02-26 15:13:51 francis Exp $
+# $Id: info_request.rb,v 1.47 2008-02-27 12:04:10 francis Exp $
 
 require 'digest/sha1'
 
@@ -257,7 +257,7 @@ public
         end
     end
     def recipient_name_and_email
-        return "FOI requests at " + self.public_body.short_name + " <" + self.recipient_email + ">"
+        return "FOI requests at " + self.public_body.short_or_long_name + " <" + self.recipient_email + ">"
     end
 
     # History of some things that have happened
diff --git a/app/models/public_body.rb b/app/models/public_body.rb
index 5fdcb85e3..9c4956b68 100644
--- a/app/models/public_body.rb
+++ b/app/models/public_body.rb
@@ -21,11 +21,11 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: public_body.rb,v 1.21 2008-02-26 15:13:51 francis Exp $
+# $Id: public_body.rb,v 1.22 2008-02-27 12:04:10 francis Exp $
 
 class PublicBody < ActiveRecord::Base
     validates_presence_of :name
-    validates_presence_of :short_name
+    validates_presence_of :url_name
     validates_presence_of :request_email
 
     has_many :info_requests
@@ -47,6 +47,28 @@ class PublicBody < ActiveRecord::Base
     acts_as_versioned
     self.non_versioned_columns << 'created_at' << 'updated_at'
 
+    # When name or short name is changed, also change the url name
+    def short_name=(short_name)
+        write_attribute(:short_name, short_name)
+        update_url_name
+    end
+    def name=(name)
+        write_attribute(:name, name)
+        update_url_name
+    end
+    def update_url_name
+        url_name = MySociety::Format.simplify_url_part(short_or_long_name)
+        write_attribute(:url_name, url_name)
+    end
+    # Return the short name if present, or else long name
+    def short_or_long_name
+        if self.short_name.nil? # can happen during construction
+            self.name
+        else
+            self.short_name.empty? ? self.name : self.short_name
+        end
+    end
+
     # Given an input string of tags, sets all tags to that string
     def tag_string=(tag_string)
         tags = tag_string.split(/\s+/).uniq
diff --git a/app/views/admin_public_body/_form.rhtml b/app/views/admin_public_body/_form.rhtml
index 0d4bf650a..1a89edf02 100644
--- a/app/views/admin_public_body/_form.rhtml
+++ b/app/views/admin_public_body/_form.rhtml
@@ -3,7 +3,7 @@
 <!--[form:public_body]-->
 <p><label for="public_body_name">Name</label><br/>
 <%= text_field 'public_body', 'name', :size => 60  %></p>
-<p><label for="public_body_short_name">Short name</label><br/>
+<p><label for="public_body_short_name">Short name (also used to make URL, leave blank for same as name)</label><br/>
 <%= text_field 'public_body', 'short_name', :size => 60  %></p>
 <p><label for="public_body_tag_string">Tags</label><br/>
 <%= text_field 'public_body', 'tag_string', :size => 40  %></p>
diff --git a/app/views/admin_public_body/edit.rhtml b/app/views/admin_public_body/edit.rhtml
index 3a1fda3e9..e7791c450 100644
--- a/app/views/admin_public_body/edit.rhtml
+++ b/app/views/admin_public_body/edit.rhtml
@@ -15,7 +15,7 @@
 <% form_tag('../destroy/' + @public_body.id.to_s) do %>
    <p>
    <%= hidden_field_tag(:public_body_id, { :value => @public_body.id } ) %>
-   <%= submit_tag "Destroy " + @public_body.short_name %> (this is permanent!)
+   <%= submit_tag "Destroy " + @public_body.short_or_long_name %> (this is permanent!)
    </p>
 <% end %>
 
diff --git a/app/views/body/_body_listing.rhtml b/app/views/body/_body_listing.rhtml
index 35ec18fb0..97660547d 100644
--- a/app/views/body/_body_listing.rhtml
+++ b/app/views/body/_body_listing.rhtml
@@ -3,8 +3,10 @@
         <%= public_body_link(public_body) %> 
         
         <br>
-        Also called <%=h public_body.short_name %>.
-        <br>
+        <% if not public_body.short_name.empty? %>
+            Also called <%=h public_body.short_name %>.
+            <br>
+        <% end %>
 
         <span class="request_listing_bottomline">
             <%= pluralize(public_body.info_requests.size, "request") %> made.
diff --git a/app/views/body/show.rhtml b/app/views/body/show.rhtml
index 57eab266e..3d57ef57e 100644
--- a/app/views/body/show.rhtml
+++ b/app/views/body/show.rhtml
@@ -3,11 +3,11 @@
 <h1><%=@title%></h1>
 
 <p class="subtitle">
-A public body in the UK, also called <%= h(@public_body.short_name) %>
+A public body in the UK<% if not @public_body.short_name.empty? %>, also called <%= h(@public_body.short_name) %><% end %>
 </p>
 
 <p>
-<%= link_to "Make new FOI request to " + @public_body.short_name, new_request_to_body_url(:public_body_id => @public_body.id.to_s)%>
+<%= link_to "Make new FOI request to " + @public_body.short_or_long_name, new_request_to_body_url(:public_body_id => @public_body.id.to_s)%>
 </p>
 
 <% if @public_body.info_requests.empty? %>
diff --git a/db/migrate/037_add_url_name.rb b/db/migrate/037_add_url_name.rb
new file mode 100644
index 000000000..dabb4889f
--- /dev/null
+++ b/db/migrate/037_add_url_name.rb
@@ -0,0 +1,18 @@
+class AddUrlName < ActiveRecord::Migration
+    def self.up
+        add_column :public_bodies, :url_name, :text
+        add_column :public_body_versions, :url_name, :text
+
+        PublicBody.find(:all).each do |public_body|
+            public_body.update_url_name
+            public_body.save!
+        end
+        add_index :public_bodies, :url_name, :unique => true
+        change_column :public_bodies, :url_name, :text, :null => false
+    end
+
+    def self.down
+        remove_column :public_bodies, :url_name
+        remove_column :public_body_versions, :url_name, :text
+    end
+end
diff --git a/db/schema.rb b/db/schema.rb
index c2d826b92..3dcab3c2f 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 => 36) do
+ActiveRecord::Schema.define(:version => 37) do
 
   create_table "incoming_messages", :force => true do |t|
     t.integer  "info_request_id",                    :null => false
@@ -77,8 +77,11 @@ ActiveRecord::Schema.define(:version => 36) do
     t.text     "last_edit_comment", :null => false
     t.datetime "created_at",        :null => false
     t.datetime "updated_at",        :null => false
+    t.text     "url_name",          :null => false
   end
 
+  add_index "public_bodies", ["url_name"], :name => "index_public_bodies_on_url_name", :unique => true
+
   create_table "public_body_tags", :force => true do |t|
     t.integer  "public_body_id", :null => false
     t.text     "name",           :null => false
@@ -97,6 +100,7 @@ ActiveRecord::Schema.define(:version => 36) do
     t.datetime "updated_at"
     t.string   "last_edit_editor"
     t.string   "last_edit_comment"
+    t.text     "url_name"
   end
 
   create_table "sessions", :force => true do |t|
diff --git a/spec/fixtures/public_bodies.yml b/spec/fixtures/public_bodies.yml
index f77fc6eb1..5a0facec9 100644
--- a/spec/fixtures/public_bodies.yml
+++ b/spec/fixtures/public_bodies.yml
@@ -8,6 +8,7 @@ geraldine_public_body:
   version: "1"
   last_edit_editor: "mark"
   short_name: TGQ
+  url_name: tgq
   created_at: 2007-10-24 10:51:01.161639
 humpadink_public_body: 
   name: "Department for Humpadinking"
@@ -19,5 +20,6 @@ humpadink_public_body:
   version: "2"
   last_edit_editor: "francis"
   short_name: DfH
+  url_name: dfh
   created_at: 2007-10-25 10:51:01.161639
 
diff --git a/spec/fixtures/public_body_versions.yml b/spec/fixtures/public_body_versions.yml
index 8efcc6846..d8bc6ea35 100644
--- a/spec/fixtures/public_body_versions.yml
+++ b/spec/fixtures/public_body_versions.yml
@@ -9,3 +9,4 @@ humpadink_public_body_old1:
   version: "1"
   last_edit_editor: "francis"
   short_name: HDINK
+  url_name: hdink
diff --git a/todo.txt b/todo.txt
index 9a94f6f03..e46e12f3e 100644
--- a/todo.txt
+++ b/todo.txt
@@ -1,3 +1,11 @@
+simple_short_name => url_name
+
+user should have url_name too
+and request!
+
+fix up comments in Format.simplify_url_part re. other places
+
+
 FOI requests to use to test it
 ==============================
 
@@ -13,7 +21,10 @@ BAILII - relationship with law courts, robots.txt ?
 Next
 ====
 
+Short name for highways agency
+
 Search and replace text "FOI" and "Freedom of Information" out the way more
+ - but put it in the title tag
 
 hidden_field_tags are all wrong - they store value1 x
 
@@ -162,6 +173,10 @@ http://www.number10.gov.uk/output/Page30.asp
 Compare against
 http://en.wikipedia.org/wiki/Departments_of_the_United_Kingdom_Government
 
+And against here:
+http://www.cabinetoffice.gov.uk/ministerial_responsibilities/departments/
+Get executive agencies from here.
+
 Skipped: Office of the Leader of the House of Commons
 
 Tricky:
@@ -184,6 +199,10 @@ Heather has some for central departments
 FOE site has email addresses of lots of bodies, e.g.
 http://community.foe.co.uk/app/tc
 
+Be sure to add:
+- all in schedule 1
+- all publically owned companies
+
 
 Visual design ideas
 ===================