diff --git a/app/controllers/members_controller.rb b/app/controllers/members_controller.rb index da580fd..5e5bee8 100644 --- a/app/controllers/members_controller.rb +++ b/app/controllers/members_controller.rb @@ -53,14 +53,12 @@ class MembersController < ApplicationController def create members = [] if params[:membership] - if params[:membership][:user_ids] - attrs = params[:membership].dup - user_ids = attrs.delete(:user_ids) - user_ids.each do |user_id| - members << Member.new(:role_ids => params[:membership][:role_ids], :user_id => user_id) - end - else - members << Member.new(:role_ids => params[:membership][:role_ids], :user_id => params[:membership][:user_id]) + user_ids = Array.wrap(params[:membership][:user_id] || params[:membership][:user_ids]) + user_ids << nil if user_ids.empty? + user_ids.each do |user_id| + member = Member.new(:project => @project, :user_id => user_id) + member.set_editable_role_ids(params[:membership][:role_ids]) + members << member end @project.members << members end @@ -84,7 +82,7 @@ class MembersController < ApplicationController def update if params[:membership] - @member.role_ids = params[:membership][:role_ids] + @member.set_editable_role_ids(params[:membership][:role_ids]) end saved = @member.save respond_to do |format| @@ -101,7 +99,7 @@ class MembersController < ApplicationController end def destroy - if request.delete? && @member.deletable? + if @member.deletable? @member.destroy end respond_to do |format| diff --git a/app/models/member.rb b/app/models/member.rb index 72a92db..037a5a0 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -29,6 +29,12 @@ class Member < ActiveRecord::Base before_destroy :set_issue_category_nil + alias :base_reload :reload + def reload(*args) + @managed_roles = nil + base_reload(*args) + end + def role end @@ -70,22 +76,52 @@ class Member < ActiveRecord::Base end end - def deletable? - member_roles.detect {|mr| mr.inherited_from}.nil? + # Set member role ids ignoring any change to roles that + # user is not allowed to manage + def set_editable_role_ids(ids, user=User.current) + ids = (ids || []).collect(&:to_i) - [0] + editable_role_ids = user.managed_roles(project).map(&:id) + untouched_role_ids = self.role_ids - editable_role_ids + touched_role_ids = ids & editable_role_ids + self.role_ids = untouched_role_ids + touched_role_ids end - def destroy - if member_roles.reload.present? - # destroying the last role will destroy another instance - # of the same Member record, #super would then trigger callbacks twice - member_roles.destroy_all - @destroyed = true - freeze + # Returns true if one of the member roles is inherited + def any_inherited_role? + member_roles.any? {|mr| mr.inherited_from} + end + + # Returns true if the member has the role and if it's inherited + def has_inherited_role?(role) + member_roles.any? {|mr| mr.role_id == role.id && mr.inherited_from.present?} + end + + # Returns true if the member's role is editable by user + def role_editable?(role, user=User.current) + if has_inherited_role?(role) + false else - super + user.managed_roles(project).include?(role) end end + # Returns true if the member is deletable by user + def deletable?(user=User.current) + if any_inherited_role? + false + else + roles & user.managed_roles(project) == roles + end + end + + # Destroys the member + def destroy + member_roles.reload.each(&:destroy_without_member_removal) + super + end + + # Returns true if the member is user or is a group + # that includes user def include?(user) if principal.is_a?(Group) !user.nil? && user.groups.include?(principal) @@ -102,6 +138,27 @@ class Member < ActiveRecord::Base end end + # Returns the roles that the member is allowed to manage + # in the project the member belongs to + def managed_roles + @managed_roles ||= begin + if principal.try(:admin?) + Role.givable.to_a + else + members_management_roles = roles.select do |role| + role.has_permission?(:manage_members) + end + if members_management_roles.empty? + [] + elsif members_management_roles.any?(&:all_roles_managed?) + Role.givable.to_a + else + members_management_roles.map(&:managed_roles).reduce(&:|) + end + end + end + end + # Creates memberships for principal with the attributes # * project_ids : one or more project ids # * role_ids : ids of the roles to give to each membership diff --git a/app/models/member_role.rb b/app/models/member_role.rb index afa5d2e..526daf9 100644 --- a/app/models/member_role.rb +++ b/app/models/member_role.rb @@ -36,10 +36,17 @@ class MemberRole < ActiveRecord::Base !inherited_from.nil? end + # Destroys the MemberRole without destroying its Member if it doesn't have + # any other roles + def destroy_without_member_removal + @member_removal = false + destroy + end + private def remove_member_if_empty - if member.roles.empty? + if @member_removal != false && member.roles.empty? member.destroy end end diff --git a/app/models/role.rb b/app/models/role.rb index 43f32b7..42dddaf 100644 --- a/app/models/role.rb +++ b/app/models/role.rb @@ -64,6 +64,10 @@ class Role < ActiveRecord::Base end has_and_belongs_to_many :custom_fields, :join_table => "#{table_name_prefix}custom_fields_roles#{table_name_suffix}", :foreign_key => "role_id" + has_and_belongs_to_many :managed_roles, :class_name => 'Role', + :join_table => "#{table_name_prefix}roles_managed_roles#{table_name_suffix}", + :association_foreign_key => "managed_role_id" + has_many :member_roles, :dependent => :destroy has_many :members, :through => :member_roles acts_as_list diff --git a/app/models/user.rb b/app/models/user.rb index 7061015..9ff7061 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -566,6 +566,15 @@ class User < Principal @visible_project_ids ||= Project.visible(self).pluck(:id) end + # Returns the roles that the user is allowed to manage for the given project + def managed_roles(project) + if admin? + Role.givable.to_a + else + membership(project).try(:managed_roles) || [] + end + end + # Returns true if user is arg or belongs to arg def is_or_belongs_to?(arg) if arg.is_a?(User) diff --git a/app/views/members/_new_form.html.erb b/app/views/members/_new_form.html.erb index 9140da7..3fe9187 100644 --- a/app/views/members/_new_form.html.erb +++ b/app/views/members/_new_form.html.erb @@ -9,7 +9,7 @@
<%= l(:label_role_plural) %> <%= toggle_checkboxes_link('.roles-selection input') %>
- <% Role.givable.all.each do |role| %> + <% User.current.managed_roles(@project).each do |role| %> <% end %>
diff --git a/app/views/projects/settings/_members.html.erb b/app/views/projects/settings/_members.html.erb index 63a4bfb..c9a91e3 100644 --- a/app/views/projects/settings/_members.html.erb +++ b/app/views/projects/settings/_members.html.erb @@ -32,9 +32,7 @@ <%= check_box_tag('membership[role_ids][]', role.id, member.roles.include?(role), :id => nil, - :disabled => member.member_roles.detect { - |mr| mr.role_id == role.id && !mr.inherited_from.nil? - } ) %> <%= role %> + :disabled => !member.role_editable?(role)) %> <%= role %>
<% end %>

diff --git a/app/views/roles/_form.html.erb b/app/views/roles/_form.html.erb index 03cbe58..8feab5d 100644 --- a/app/views/roles/_form.html.erb +++ b/app/views/roles/_form.html.erb @@ -16,6 +16,28 @@

<%= f.select :users_visibility, Role::USERS_VISIBILITY_OPTIONS.collect {|v| [l(v.last), v.first]} %>

+ <% unless @role.builtin? %> +

+ + + + <% Role.givable.sorted.each do |role| %> + + <% end %> + <%= hidden_field_tag 'role[managed_role_ids][]', '' %> + <% end %> + <% if @role.new_record? && @roles.any? %>

<%= select_tag(:copy_workflow_from, content_tag("option") + options_from_collection_for_select(@roles, :id, :name, params[:copy_workflow_from] || @copy_from.try(:id))) %>

@@ -29,7 +51,8 @@
<%= mod.blank? ? l(:label_project) : l_or_humanize(mod, :prefix => 'project_module_') %> <% perms_by_module[mod].each do |permission| %> <% end %> @@ -38,3 +61,11 @@
<%= check_all_links 'permissions' %> <%= hidden_field_tag 'role[permissions][]', '' %> + +<%= javascript_tag do %> +$(document).ready(function(){ + $("#role_permissions_manage_members").change(function(){ + $("#manage_members_options").toggle($(this).is(":checked")); + }).change(); +}); +<% end %> diff --git a/db/migrate/20150528084820_add_roles_all_roles_managed.rb b/db/migrate/20150528084820_add_roles_all_roles_managed.rb new file mode 100644 index 0000000..c4e2b9b --- /dev/null +++ b/db/migrate/20150528084820_add_roles_all_roles_managed.rb @@ -0,0 +1,5 @@ +class AddRolesAllRolesManaged < ActiveRecord::Migration + def change + add_column :roles, :all_roles_managed, :boolean, :default => true, :null => false + end +end diff --git a/db/migrate/20150528092912_create_roles_managed_roles.rb b/db/migrate/20150528092912_create_roles_managed_roles.rb new file mode 100644 index 0000000..94e5ee2 --- /dev/null +++ b/db/migrate/20150528092912_create_roles_managed_roles.rb @@ -0,0 +1,8 @@ +class CreateRolesManagedRoles < ActiveRecord::Migration + def change + create_table :roles_managed_roles, :id => false do |t| + t.integer :role_id, :null => false + t.integer :managed_role_id, :null => false + end + end +end diff --git a/db/migrate/20150528093249_add_unique_index_on_roles_managed_roles.rb b/db/migrate/20150528093249_add_unique_index_on_roles_managed_roles.rb new file mode 100644 index 0000000..74dc64b --- /dev/null +++ b/db/migrate/20150528093249_add_unique_index_on_roles_managed_roles.rb @@ -0,0 +1,5 @@ +class AddUniqueIndexOnRolesManagedRoles < ActiveRecord::Migration + def change + add_index :roles_managed_roles, [:role_id, :managed_role_id], :unique => true + end +end diff --git a/test/functional/members_controller_test.rb b/test/functional/members_controller_test.rb index c1f4350..5bad287 100644 --- a/test/functional/members_controller_test.rb +++ b/test/functional/members_controller_test.rb @@ -30,6 +30,20 @@ class MembersControllerTest < ActionController::TestCase assert_response :success end + def test_new_should_propose_managed_roles_only + role = Role.find(1) + role.update! :all_roles_managed => false + role.managed_roles = Role.where(:id => [2, 3]).to_a + + get :new, :project_id => 1 + assert_response :success + assert_select 'div.roles-selection' do + assert_select 'label', :text => 'Manager', :count => 0 + assert_select 'label', :text => 'Developer' + assert_select 'label', :text => 'Reporter' + end + end + def test_xhr_new xhr :get, :new, :project_id => 1 assert_response :success @@ -52,6 +66,29 @@ class MembersControllerTest < ActionController::TestCase assert User.find(7).member_of?(Project.find(1)) end + def test_create_should_ignore_unmanaged_roles + role = Role.find(1) + role.update! :all_roles_managed => false + role.managed_roles = Role.where(:id => [2, 3]).to_a + + assert_difference 'Member.count' do + post :create, :project_id => 1, :membership => {:role_ids => [1, 2], :user_id => 7} + end + member = Member.order(:id => :desc).first + assert_equal [2], member.role_ids + end + + def test_create_should_be_allowed_for_admin_without_role + User.find(1).members.delete_all + @request.session[:user_id] = 1 + + assert_difference 'Member.count' do + post :create, :project_id => 1, :membership => {:role_ids => [1, 2], :user_id => 7} + end + member = Member.order(:id => :desc).first + assert_equal [1, 2], member.role_ids + end + def test_xhr_create assert_difference 'Member.count', 3 do xhr :post, :create, :project_id => 1, :membership => {:role_ids => [1], :user_ids => [7, 8, 9]} @@ -75,14 +112,34 @@ class MembersControllerTest < ActionController::TestCase assert_match /alert/, response.body, "Alert message not sent" end - def test_edit + def test_update assert_no_difference 'Member.count' do put :update, :id => 2, :membership => {:role_ids => [1], :user_id => 3} end assert_redirected_to '/projects/ecookbook/settings/members' end - def test_xhr_edit + def test_update_should_not_add_unmanaged_roles + role = Role.find(1) + role.update! :all_roles_managed => false + role.managed_roles = Role.where(:id => [2, 3]).to_a + member = Member.create!(:user => User.find(9), :role_ids => [3], :project_id => 1) + + put :update, :id => member.id, :membership => {:role_ids => [1, 2, 3]} + assert_equal [2, 3], member.reload.role_ids.sort + end + + def test_update_should_not_remove_unmanaged_roles + role = Role.find(1) + role.update! :all_roles_managed => false + role.managed_roles = Role.where(:id => [2, 3]).to_a + member = Member.create!(:user => User.find(9), :role_ids => [1, 3], :project_id => 1) + + put :update, :id => member.id, :membership => {:role_ids => [2]} + assert_equal [1, 2], member.reload.role_ids.sort + end + + def test_xhr_update assert_no_difference 'Member.count' do xhr :put, :update, :id => 2, :membership => {:role_ids => [1], :user_id => 3} assert_response :success @@ -103,6 +160,28 @@ class MembersControllerTest < ActionController::TestCase assert !User.find(3).member_of?(Project.find(1)) end + def test_destroy_should_fail_with_unmanaged_roles + role = Role.find(1) + role.update! :all_roles_managed => false + role.managed_roles = Role.where(:id => [2, 3]).to_a + member = Member.create!(:user => User.find(9), :role_ids => [1, 3], :project_id => 1) + + assert_no_difference 'Member.count' do + delete :destroy, :id => member.id + end + end + + def test_destroy_should_succeed_with_managed_roles_only + role = Role.find(1) + role.update! :all_roles_managed => false + role.managed_roles = Role.where(:id => [2, 3]).to_a + member = Member.create!(:user => User.find(9), :role_ids => [3], :project_id => 1) + + assert_difference 'Member.count', -1 do + delete :destroy, :id => member.id + end + end + def test_xhr_destroy assert_difference 'Member.count', -1 do xhr :delete, :destroy, :id => 2 diff --git a/test/unit/member_test.rb b/test/unit/member_test.rb index 2ed6554..221cae4 100644 --- a/test/unit/member_test.rb +++ b/test/unit/member_test.rb @@ -159,4 +159,35 @@ class MemberTest < ActiveSupport::TestCase assert_equal -1, a <=> b assert_equal 1, b <=> a end + + def test_managed_roles_should_return_all_roles_for_role_with_all_roles_managed + member = Member.new + member.roles << Role.generate!(:permissions => [:manage_members], :all_roles_managed => true) + assert_equal Role.givable.all, member.managed_roles + end + + def test_managed_roles_should_return_all_roles_for_admins + member = Member.new(:user => User.find(1)) + member.roles << Role.generate! + assert_equal Role.givable.all, member.managed_roles + end + + def test_managed_roles_should_return_limited_roles_for_role_without_all_roles_managed + member = Member.new + member.roles << Role.generate!(:permissions => [:manage_members], :all_roles_managed => false, :managed_role_ids => [2, 3]) + assert_equal [2, 3], member.managed_roles.map(&:id).sort + end + + def test_managed_roles_should_cumulated_managed_roles + member = Member.new + member.roles << Role.generate!(:permissions => [:manage_members], :all_roles_managed => false, :managed_role_ids => [3]) + member.roles << Role.generate!(:permissions => [:manage_members], :all_roles_managed => false, :managed_role_ids => [2]) + assert_equal [2, 3], member.managed_roles.map(&:id).sort + end + + def test_managed_roles_should_return_no_roles_for_role_without_permission + member = Member.new + member.roles << Role.generate!(:all_roles_managed => true) + assert_equal [], member.managed_roles + end end