Resourcified roles.

git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@8145 e93f8b46-1217-0410-a6f0-8f06a7374b81

10 files changed, 81 insertions, 56 deletions

diff --git a/app/controllers/roles_controller.rb b/app/controllers/roles_controller.rb
index c0713cb82..b74404470 100644
--- a/app/controllers/roles_controller.rb
+++ b/app/controllers/roles_controller.rb
@@ -19,9 +19,8 @@ class RolesController < ApplicationController
   layout 'admin'
 
   before_filter :require_admin
+  before_filter :find_role, :only => [:edit, :update, :destroy]
 
-  verify :method => :post, :only => [ :destroy ],
-         :redirect_to => { :action => :index }
 
   def index
     @role_pages, @roles = paginate :roles, :per_page => 25, :order => 'builtin, position'
@@ -31,6 +30,11 @@ class RolesController < ApplicationController
   def new
     # Prefills the form with 'Non member' role permissions
     @role = Role.new(params[:role] || {:permissions => Role.non_member.permissions})
+    @roles = Role.all
+  end
+
+  def create
+    @role = Role.new(params[:role])
     if request.post? && @role.save
       # workflow copy
       if !params[:copy_workflow_from].blank? && (copy_from = Role.find_by_id(params[:copy_workflow_from]))
@@ -39,23 +43,25 @@ class RolesController < ApplicationController
       flash[:notice] = l(:notice_successful_create)
       redirect_to :action => 'index'
     else
-      @permissions = @role.setable_permissions
-      @roles = Role.find :all, :order => 'builtin, position'
+      @roles = Role.all
+      render :action => 'new'
     end
   end
 
   def edit
-    @role = Role.find(params[:id])
-    if request.post? and @role.update_attributes(params[:role])
+  end
+
+  def update
+    if request.put? and @role.update_attributes(params[:role])
       flash[:notice] = l(:notice_successful_update)
       redirect_to :action => 'index'
     else
-      @permissions = @role.setable_permissions
+      render :action => 'edit'
     end
   end
 
+  verify :method => :delete, :only => :destroy, :redirect_to => { :action => :index }
   def destroy
-    @role = Role.find(params[:id])
     @role.destroy
     redirect_to :action => 'index'
   rescue
@@ -63,7 +69,7 @@ class RolesController < ApplicationController
     redirect_to :action => 'index'
   end
 
-  def report
+  def permissions
     @roles = Role.find(:all, :order => 'builtin, position')
     @permissions = Redmine::AccessControl.permissions.select { |p| !p.public? }
     if request.post?
@@ -75,4 +81,12 @@ class RolesController < ApplicationController
       redirect_to :action => 'index'
     end
   end
+
+  private
+
+  def find_role
+    @role = Role.find(params[:id])
+  rescue ActiveRecord::RecordNotFound
+    render_404
+  end
 end
diff --git a/app/models/role.rb b/app/models/role.rb
index 9d9d8ae94..e0c0fef1c 100644
--- a/app/models/role.rb
+++ b/app/models/role.rb
@@ -26,6 +26,7 @@ class Role < ActiveRecord::Base
     ['own', :label_issues_visibility_own]
   ]
 
+  default_scope :order => 'builtin, position'
   named_scope :givable, { :conditions => "builtin = 0", :order => 'position' }
   named_scope :builtin, lambda { |*args|
     compare = 'not' if args.first == true
diff --git a/app/views/roles/_form.html.erb b/app/views/roles/_form.html.erb
index 17b012313..45f8b0d86 100644
--- a/app/views/roles/_form.html.erb
+++ b/app/views/roles/_form.html.erb
@@ -14,7 +14,7 @@
 
 <h3><%= l(:label_permissions) %></h3>
 <div class="box tabular" id="permissions">
-<% perms_by_module = @permissions.group_by {|p| p.project_module.to_s} %>
+<% perms_by_module = @role.setable_permissions.group_by {|p| p.project_module.to_s} %>
 <% perms_by_module.keys.sort.each do |mod| %>
     <fieldset><legend><%= mod.blank? ? l(:label_project) : l_or_humanize(mod, :prefix => 'project_module_') %></legend>
     <% perms_by_module[mod].each do |permission| %>
diff --git a/app/views/roles/edit.html.erb b/app/views/roles/edit.html.erb
index 2b1bd793b..3bb07c08f 100644
--- a/app/views/roles/edit.html.erb
+++ b/app/views/roles/edit.html.erb
@@ -1,6 +1,6 @@
-<h2><%= link_to l(:label_role_plural), :controller => 'roles', :action => 'index' %> &#187; <%=h @role.name %></h2>
+<h2><%= link_to l(:label_role_plural), roles_path %> &#187; <%=h @role.name %></h2>
 
-<% labelled_form_for :role, @role, :url => { :action => 'edit' }, :html => {:id => 'role_form'} do |f| %>
+<% labelled_form_for @role do |f| %>
 <%= render :partial => 'form', :locals => { :f => f } %>
 <%= submit_tag l(:button_save) %>
 <% end %>
diff --git a/app/views/roles/index.html.erb b/app/views/roles/index.html.erb
index 3edd4672d..9d3c46c14 100644
--- a/app/views/roles/index.html.erb
+++ b/app/views/roles/index.html.erb
@@ -1,5 +1,5 @@
 <div class="contextual">
-<%= link_to l(:label_role_new), {:action => 'new'}, :class => 'icon icon-add' %>
+<%= link_to l(:label_role_new), new_role_path, :class => 'icon icon-add' %>
 </div>
 
 <h2><%=l(:label_role_plural)%></h2>
@@ -13,15 +13,15 @@
   <tbody>
 <% for role in @roles %>
   <tr class="<%= cycle("odd", "even") %>">
-  <td><%= content_tag(role.builtin? ? 'em' : 'span', link_to(h(role.name), :action => 'edit', :id => role)) %></td>
+  <td><%= content_tag(role.builtin? ? 'em' : 'span', link_to(h(role.name), edit_role_path(role))) %></td>
   <td align="center" style="width:15%;">
   <% unless role.builtin? %>
-    <%= reorder_links('role', {:action => 'edit', :id => role}) %>
+    <%= reorder_links('role', {:action => 'update', :id => role}, :put) %>
   <% end %>
   </td>
   <td class="buttons">
-    <%= link_to(l(:button_delete), { :action => 'destroy', :id => role },
-                                  :method => :post,
+    <%= link_to(l(:button_delete), role_path(role),
+                                  :method => :delete,
                                   :confirm => l(:text_are_you_sure),
                                   :class => 'icon icon-del') unless role.builtin? %>
   </td>
@@ -32,6 +32,6 @@
 
 <p class="pagination"><%= pagination_links_full @role_pages %></p>
 
-<p><%= link_to l(:label_permissions_report), :action => 'report' %></p>
+<p><%= link_to l(:label_permissions_report), :action => 'permissions' %></p>
 
 <% html_title(l(:label_role_plural)) -%>
diff --git a/app/views/roles/new.html.erb b/app/views/roles/new.html.erb
index f6a872cb2..77f31a7c0 100644
--- a/app/views/roles/new.html.erb
+++ b/app/views/roles/new.html.erb
@@ -1,6 +1,6 @@
-<h2><%= link_to l(:label_role_plural), :controller => 'roles', :action => 'index' %> &#187; <%=l(:label_role_new)%></h2>
+<h2><%= link_to l(:label_role_plural), roles_path %> &#187; <%=l(:label_role_new)%></h2>
 
-<% labelled_form_for :role, @role, :url => { :action => 'new' }, :html => {:id => 'role_form'} do |f| %>
+<% labelled_form_for @role do |f| %>
 <%= render :partial => 'form', :locals => { :f => f } %>
 <%= submit_tag l(:button_create) %>
 <% end %>
diff --git a/app/views/roles/report.html.erb b/app/views/roles/permissions.html.erb
index 052f7e267..32703e791 100644
--- a/app/views/roles/report.html.erb
+++ b/app/views/roles/permissions.html.erb
@@ -1,6 +1,6 @@
-<h2><%= link_to l(:label_role_plural), :controller => 'roles', :action => 'index' %> &#187; <%=l(:label_permissions_report)%></h2>
+<h2><%= link_to l(:label_role_plural), roles_path %> &#187; <%=l(:label_permissions_report)%></h2>
 
-<% form_tag({:action => 'report'}, :id => 'permissions_form') do %>
+<% form_tag(permissions_roles_path, :id => 'permissions_form') do %>
 <%= hidden_field_tag 'permissions[0]', '', :id => nil %>
 <div class="autoscroll">
 <table class="list">
diff --git a/config/routes.rb b/config/routes.rb
index f17ad6f00..46b4654e4 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -185,6 +185,7 @@ ActionController::Routing::Routes.draw do |map|
   map.resources :trackers, :except => :show
   map.resources :issue_statuses, :except => :show, :collection => {:update_issue_done_ratio => :post}
   map.resources :custom_fields, :except => :show
+  map.resources :roles, :except => :show, :collection => {:permissions => [:get, :post]}
 
   #left old routes at the bottom for backwards compat
   map.connect 'boards/:board_id/topics/:action/:id', :controller => 'messages'
diff --git a/test/functional/roles_controller_test.rb b/test/functional/roles_controller_test.rb
index 23d1b3657..b0fdb4368 100644
--- a/test/functional/roles_controller_test.rb
+++ b/test/functional/roles_controller_test.rb
@@ -16,23 +16,16 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
 
 require File.expand_path('../../test_helper', __FILE__)
-require 'roles_controller'
-
-# Re-raise errors caught by the controller.
-class RolesController; def rescue_action(e) raise e end; end
 
 class RolesControllerTest < ActionController::TestCase
   fixtures :roles, :users, :members, :member_roles, :workflows, :trackers
 
   def setup
-    @controller = RolesController.new
-    @request    = ActionController::TestRequest.new
-    @response   = ActionController::TestResponse.new
     User.current = nil
     @request.session[:user_id] = 1 # admin
   end
 
-  def test_get_index
+  def test_index
     get :index
     assert_response :success
     assert_template 'index'
@@ -40,18 +33,18 @@ class RolesControllerTest < ActionController::TestCase
     assert_not_nil assigns(:roles)
     assert_equal Role.find(:all, :order => 'builtin, position'), assigns(:roles)
 
-    assert_tag :tag => 'a', :attributes => { :href => '/roles/edit/1' },
+    assert_tag :tag => 'a', :attributes => { :href => '/roles/1/edit' },
                             :content => 'Manager'
   end
 
-  def test_get_new
+  def test_new
     get :new
     assert_response :success
     assert_template 'new'
   end
 
-  def test_post_new_with_validaton_failure
-    post :new, :role => {:name => '',
+  def test_create_with_validaton_failure
+    post :create, :role => {:name => '',
                          :permissions => ['add_issues', 'edit_issues', 'log_time', ''],
                          :assignable => '0'}
 
@@ -60,8 +53,8 @@ class RolesControllerTest < ActionController::TestCase
     assert_tag :tag => 'div', :attributes => { :id => 'errorExplanation' }
   end
 
-  def test_post_new_without_workflow_copy
-    post :new, :role => {:name => 'RoleWithoutWorkflowCopy',
+  def test_create_without_workflow_copy
+    post :create, :role => {:name => 'RoleWithoutWorkflowCopy',
                          :permissions => ['add_issues', 'edit_issues', 'log_time', ''],
                          :assignable => '0'}
 
@@ -72,8 +65,8 @@ class RolesControllerTest < ActionController::TestCase
     assert !role.assignable?
   end
 
-  def test_post_new_with_workflow_copy
-    post :new, :role => {:name => 'RoleWithWorkflowCopy',
+  def test_create_with_workflow_copy
+    post :create, :role => {:name => 'RoleWithWorkflowCopy',
                          :permissions => ['add_issues', 'edit_issues', 'log_time', ''],
                          :assignable => '0'},
                :copy_workflow_from => '1'
@@ -84,15 +77,15 @@ class RolesControllerTest < ActionController::TestCase
     assert_equal Role.find(1).workflows.size, role.workflows.size
   end
 
-  def test_get_edit
+  def test_edit
     get :edit, :id => 1
     assert_response :success
     assert_template 'edit'
     assert_equal Role.find(1), assigns(:role)
   end
 
-  def test_post_edit
-    post :edit, :id => 1,
+  def test_update
+    put :update, :id => 1,
                 :role => {:name => 'Manager',
                           :permissions => ['edit_project', ''],
                           :assignable => '0'}
@@ -102,26 +95,31 @@ class RolesControllerTest < ActionController::TestCase
     assert_equal [:edit_project], role.permissions
   end
 
+  def test_update_with_failure
+    put :update, :id => 1, :role => {:name => ''}
+    assert_response :success
+    assert_template 'edit'
+  end
+
   def test_destroy
-    r = Role.new(:name => 'ToBeDestroyed', :permissions => [:view_wiki_pages])
-    assert r.save
+    r = Role.create!(:name => 'ToBeDestroyed', :permissions => [:view_wiki_pages])
 
-    post :destroy, :id => r
+    delete :destroy, :id => r
     assert_redirected_to '/roles'
     assert_nil Role.find_by_id(r.id)
   end
 
   def test_destroy_role_in_use
-    post :destroy, :id => 1
+    delete :destroy, :id => 1
     assert_redirected_to '/roles'
-    assert flash[:error] == 'This role is in use and cannot be deleted.'
+    assert_equal 'This role is in use and cannot be deleted.', flash[:error] 
     assert_not_nil Role.find_by_id(1)
   end
 
-  def test_get_report
-    get :report
+  def test_get_permissions
+    get :permissions
     assert_response :success
-    assert_template 'report'
+    assert_template 'permissions'
 
     assert_not_nil assigns(:roles)
     assert_equal Role.find(:all, :order => 'builtin, position'), assigns(:roles)
@@ -137,8 +135,8 @@ class RolesControllerTest < ActionController::TestCase
                                                  :checked => nil }
   end
 
-  def test_post_report
-    post :report, :permissions => { '0' => '', '1' => ['edit_issues'], '3' => ['add_issues', 'delete_issues']}
+  def test_post_permissions
+    post :permissions, :permissions => { '0' => '', '1' => ['edit_issues'], '3' => ['add_issues', 'delete_issues']}
     assert_redirected_to '/roles'
 
     assert_equal [:edit_issues], Role.find(1).permissions
@@ -147,33 +145,33 @@ class RolesControllerTest < ActionController::TestCase
   end
 
   def test_clear_all_permissions
-    post :report, :permissions => { '0' => '' }
+    post :permissions, :permissions => { '0' => '' }
     assert_redirected_to '/roles'
     assert Role.find(1).permissions.empty?
   end
 
   def test_move_highest
-    post :edit, :id => 3, :role => {:move_to => 'highest'}
+    put :update, :id => 3, :role => {:move_to => 'highest'}
     assert_redirected_to '/roles'
     assert_equal 1, Role.find(3).position
   end
 
   def test_move_higher
     position = Role.find(3).position
-    post :edit, :id => 3, :role => {:move_to => 'higher'}
+    put :update, :id => 3, :role => {:move_to => 'higher'}
     assert_redirected_to '/roles'
     assert_equal position - 1, Role.find(3).position
   end
 
   def test_move_lower
     position = Role.find(2).position
-    post :edit, :id => 2, :role => {:move_to => 'lower'}
+    put :update, :id => 2, :role => {:move_to => 'lower'}
     assert_redirected_to '/roles'
     assert_equal position + 1, Role.find(2).position
   end
 
   def test_move_lowest
-    post :edit, :id => 2, :role => {:move_to => 'lowest'}
+    put :update, :id => 2, :role => {:move_to => 'lowest'}
     assert_redirected_to '/roles'
     assert_equal Role.count, Role.find(2).position
   end
diff --git a/test/integration/routing_test.rb b/test/integration/routing_test.rb
index d196c0873..552aa52b2 100644
--- a/test/integration/routing_test.rb
+++ b/test/integration/routing_test.rb
@@ -284,6 +284,17 @@ class RoutingTest < ActionController::IntegrationTest
     should_route :post, "/projects/redmine/repository/edit", :controller => 'repositories', :action => 'edit', :id => 'redmine'
   end
 
+  context "roles" do
+    should_route :get, "/roles", :controller => 'roles', :action => 'index'
+    should_route :get, "/roles/new", :controller => 'roles', :action => 'new'
+    should_route :post, "/roles", :controller => 'roles', :action => 'create'
+    should_route :get, "/roles/2/edit", :controller => 'roles', :action => 'edit', :id => 2
+    should_route :put, "/roles/2", :controller => 'roles', :action => 'update', :id => 2
+    should_route :delete, "/roles/2", :controller => 'roles', :action => 'destroy', :id => 2
+    should_route :get, "/roles/permissions", :controller => 'roles', :action => 'permissions'
+    should_route :post, "/roles/permissions", :controller => 'roles', :action => 'permissions'
+  end
+
   context "timelogs (global)" do
     should_route :get, "/time_entries", :controller => 'timelog', :action => 'index'
     should_route :get, "/time_entries.csv", :controller => 'timelog', :action => 'index', :format => 'csv'