lets attachments be resourceful

* still way to much code for the route, even more than when using match but I hope that using resources will pay of
* should allow the id to be the filename
* redirects old routes to new ones

config/routes.rb

@@ -236,9 +236,19 @@ OpenProject::Application.routes.draw do
       match '/projects/:id/repository/:action', :via => :post
     end
 
-    match '/attachments/:id' => 'attachments#show', :id => /\d+/
-    match '/attachments/:id/:filename' => 'attachments#show', :id => /\d+/, :filename => /.*/
-    match '/attachments/download/:id/:filename' => 'attachments#download', :id => /\d+/, :filename => /.*/
+    resources :attachments, :only => [:show], :format => false do
+      member do
+        scope :via => :get,  :constraints => { :id => /\d+/, :filename => /[^\/]*/ } do
+          match 'download(/:filename)' => 'attachments#download', :as => 'download'
+          match ':filename' => 'attachments#show'
+        end
+      end
+    end
+    # redirect for backwards compatibility
+    scope :constraints => { :id => /\d+/, :filename => /[^\/]*/ } do
+      match "/attachments/download/:id/:filename" => redirect("/attachments/%{id}/download/%{filename}"), :format => false
+      match "/attachments/download/:id" => redirect("/attachments/%{id}/download"), :format => false
+    end
 
     resources :groups
 

test/integration/routing_test.rb

@@ -36,10 +36,29 @@ class RoutingTest < ActionController::IntegrationTest
   end
 
   context "attachments" do
-    should_route :get, "/attachments/1", :controller => 'attachments', :action => 'show', :id => '1'
-    should_route :get, "/attachments/1/filename.ext", :controller => 'attachments', :action => 'show', :id => '1', :filename => 'filename.ext'
-    should_route :get, "/attachments/download/1", :controller => 'attachments', :action => 'download', :id => '1'
-    should_route :get, "/attachments/download/1/filename.ext", :controller => 'attachments', :action => 'download', :id => '1', :filename => 'filename.ext'
+    should route(:get, "/attachments/1").to( :controller => 'attachments',
+                                             :action => 'show',
+                                             :id => '1')
+    should route(:get, "/attachments/1/filename.ext").to( :controller => 'attachments',
+                                                          :action => 'show',
+                                                          :id => '1',
+                                                          :filename => 'filename.ext' )
+    should route(:get, "/attachments/1/download").to( :controller => 'attachments',
+                                                      :action => 'download',
+                                                      :id => '1' )
+    should route(:get, "/attachments/1/download/filename.ext").to( :controller => 'attachments',
+                                                                   :action => 'download',
+                                                                   :id => '1',
+                                                                   :filename => 'filename.ext' )
+    should "redirect /atttachments/download/1 to /attachments/1/download" do
+      get '/attachments/download/1'
+      assert_redirected_to '/attachments/1/download'
+    end
+
+    should "redirect /atttachments/download/1/filename.ext to /attachments/1/download/filename.ext" do
+      get '/attachments/download/1/filename.ext'
+      assert_redirected_to '/attachments/1/download/filename.ext'
+    end
   end
 
   context "boards" do