From 532a76f78c917d4391f4a8ecce9f8201b041d57d Mon Sep 17 00:00:00 2001 From: Jean-Philippe Lang Date: Fri, 9 Dec 2011 23:29:58 +0000 Subject: [PATCH] Resourcified roles. git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@8145 e93f8b46-1217-0410-a6f0-8f06a7374b81 --- app/controllers/roles_controller.rb | 32 ++++++--- app/models/role.rb | 1 + app/views/roles/_form.html.erb | 2 +- app/views/roles/edit.html.erb | 4 +- app/views/roles/index.html.erb | 12 ++-- app/views/roles/new.html.erb | 4 +- .../{report.html.erb => permissions.html.erb} | 4 +- config/routes.rb | 1 + test/functional/roles_controller_test.rb | 66 +++++++++---------- test/integration/routing_test.rb | 11 ++++ 10 files changed, 81 insertions(+), 56 deletions(-) rename app/views/roles/{report.html.erb => permissions.html.erb} (90%) 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 @@

<%= l(:label_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| %>
<%= mod.blank? ? l(:label_project) : l_or_humanize(mod, :prefix => 'project_module_') %> <% 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 @@ -

<%= link_to l(:label_role_plural), :controller => 'roles', :action => 'index' %> » <%=h @role.name %>

+

<%= link_to l(:label_role_plural), roles_path %> » <%=h @role.name %>

-<% 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 @@
-<%= 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' %>

<%=l(:label_role_plural)%>

@@ -13,15 +13,15 @@ <% for role in @roles %> "> - <%= content_tag(role.builtin? ? 'em' : 'span', link_to(h(role.name), :action => 'edit', :id => role)) %> + <%= content_tag(role.builtin? ? 'em' : 'span', link_to(h(role.name), edit_role_path(role))) %> <% unless role.builtin? %> - <%= reorder_links('role', {:action => 'edit', :id => role}) %> + <%= reorder_links('role', {:action => 'update', :id => role}, :put) %> <% end %> - <%= 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? %> @@ -32,6 +32,6 @@

<%= pagination_links_full @role_pages %>

-

<%= link_to l(:label_permissions_report), :action => 'report' %>

+

<%= link_to l(:label_permissions_report), :action => 'permissions' %>

<% 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 @@ -

<%= link_to l(:label_role_plural), :controller => 'roles', :action => 'index' %> » <%=l(:label_role_new)%>

+

<%= link_to l(:label_role_plural), roles_path %> » <%=l(:label_role_new)%>

-<% 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 similarity index 90% rename from app/views/roles/report.html.erb rename to 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 @@ -

<%= link_to l(:label_role_plural), :controller => 'roles', :action => 'index' %> » <%=l(:label_permissions_report)%>

+

<%= link_to l(:label_role_plural), roles_path %> » <%=l(:label_permissions_report)%>

-<% form_tag({:action => 'report'}, :id => 'permissions_form') do %> +<% form_tag(permissions_roles_path, :id => 'permissions_form') do %> <%= hidden_field_tag 'permissions[0]', '', :id => nil %>
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' -- 2.39.5