diff options
27 files changed, 136 insertions, 114 deletions
diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index a2201f732e6..89b395786b3 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -20,9 +20,9 @@ class Admin::GroupsController < Admin::ApplicationController def create @group = Group.new(params[:group]) @group.path = @group.name.dup.parameterize if @group.name - @group.owner = current_user if @group.save + @group.add_owner(current_user) redirect_to [:admin, @group], notice: 'Group was successfully created.' else render "new" @@ -30,14 +30,7 @@ class Admin::GroupsController < Admin::ApplicationController end def update - group_params = params[:group].dup - owner_id =group_params.delete(:owner_id) - - if owner_id - @group.change_owner(User.find(owner_id)) - end - - if @group.update_attributes(group_params) + if @group.update_attributes(params[:group]) redirect_to [:admin, @group], notice: 'Group was successfully updated.' else render "edit" diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 9dc9afe9e67..f80167da4cb 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -21,9 +21,9 @@ class GroupsController < ApplicationController def create @group = Group.new(params[:group]) @group.path = @group.name.dup.parameterize if @group.name - @group.owner = current_user if @group.save + @group.add_owner(current_user) redirect_to @group, notice: 'Group was successfully created.' else render action: "new" @@ -73,15 +73,7 @@ class GroupsController < ApplicationController end def update - group_params = params[:group].dup - owner_id = group_params.delete(:owner_id) - - if owner_id - @group.owner = User.find(owner_id) - @group.save - end - - if @group.update_attributes(group_params) + if @group.update_attributes(params[:group]) redirect_to @group, notice: 'Group was successfully updated.' else render action: "edit" diff --git a/app/controllers/profiles/groups_controller.rb b/app/controllers/profiles/groups_controller.rb index e276d07b489..378ff6bcf34 100644 --- a/app/controllers/profiles/groups_controller.rb +++ b/app/controllers/profiles/groups_controller.rb @@ -8,8 +8,8 @@ class Profiles::GroupsController < ApplicationController def leave @users_group = group.users_groups.where(user_id: current_user.id).first - if group.owner == current_user - redirect_to(profile_groups_path, alert: "You can't leave group. You must transfer it to another owner before leaving.") + if group.last_owner?(current_user) + redirect_to(profile_groups_path, alert: "You can't leave group. You must add at least one more owner to it.") else @users_group.destroy redirect_to(profile_groups_path, info: "You left #{group.name} group.") diff --git a/app/controllers/users_groups_controller.rb b/app/controllers/users_groups_controller.rb index df13b86fdcd..749da1e1413 100644 --- a/app/controllers/users_groups_controller.rb +++ b/app/controllers/users_groups_controller.rb @@ -19,7 +19,7 @@ class UsersGroupsController < ApplicationController def destroy @users_group = @group.users_groups.find(params[:id]) - @users_group.destroy unless @users_group.user == @group.owner + @users_group.destroy respond_to do |format| format.html { redirect_to members_group_path(@group), notice: 'User was successfully removed from group.' } diff --git a/app/models/ability.rb b/app/models/ability.rb index ad070dad296..85476089145 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -79,7 +79,7 @@ class Ability rules << project_admin_rules end - if project.group && project.group.owners.include?(user) + if project.group && project.group.has_owner?(user) rules << project_admin_rules end @@ -159,7 +159,7 @@ class Ability end # Only group owner and administrators can manage group - if group.owners.include?(user) || user.admin? + if group.has_owner?(user) || user.admin? rules << [ :manage_group, :manage_namespace diff --git a/app/models/group.rb b/app/models/group.rb index 0b36c934375..0cc3a3a0f41 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -16,14 +16,12 @@ class Group < Namespace has_many :users_groups, dependent: :destroy has_many :users, through: :users_groups - after_create :add_owner - def human_name name end def owners - @owners ||= (users_groups.owners.map(&:user) << owner).uniq + @owners ||= users_groups.owners.map(&:user) end def add_users(user_ids, group_access) @@ -36,20 +34,19 @@ class Group < Namespace self.users_groups.create(user_id: user.id, group_access: group_access) end - def change_owner(user) - self.owner = user - membership = users_groups.where(user_id: user.id).first + def add_owner(user) + self.add_user(user, UsersGroup::OWNER) + end - if membership - membership.update_attributes(group_access: UsersGroup::OWNER) - else - add_owner - end + def has_owner?(user) + owners.include?(user) end - private + def last_owner?(user) + has_owner?(user) && owners.size == 1 + end - def add_owner - self.add_users([owner.id], UsersGroup::OWNER) + def members + users_groups end end diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 18959166931..6ac94c06604 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -20,7 +20,7 @@ class Namespace < ActiveRecord::Base has_many :projects, dependent: :destroy belongs_to :owner, class_name: "User" - validates :owner, presence: true + validates :owner, presence: true, unless: ->(n) { n.type == "Group" } validates :name, presence: true, uniqueness: true, length: { within: 0..255 }, format: { with: Gitlab::Regex.name_regex, diff --git a/app/models/project.rb b/app/models/project.rb index 63cd3505ec4..c4f4b06713d 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -249,10 +249,10 @@ class Project < ActiveRecord::Base end def owner - if namespace - namespace_owner + if group + group else - creator + namespace.try(:owner) end end @@ -276,10 +276,6 @@ class Project < ActiveRecord::Base end end - def namespace_owner - namespace.try(:owner) - end - def path_with_namespace if namespace namespace.path + '/' + path diff --git a/app/models/user.rb b/app/models/user.rb index ea004e4c9a7..f1f93eadc1a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -135,7 +135,7 @@ class User < ActiveRecord::Base # Remove user from all groups user.users_groups.find_each do |membership| # skip owned resources - next if membership.group.owners.include?(user) + next if membership.group.last_owner?(user) return false unless membership.destroy end diff --git a/app/services/system_hooks_service.rb b/app/services/system_hooks_service.rb index c872b27ba39..39aec943f75 100644 --- a/app/services/system_hooks_service.rb +++ b/app/services/system_hooks_service.rb @@ -23,13 +23,15 @@ class SystemHooksService case model when Project + owner = model.owner + data.merge!({ name: model.name, path: model.path, path_with_namespace: model.path_with_namespace, project_id: model.id, - owner_name: model.owner.name, - owner_email: model.owner.email + owner_name: owner.name, + owner_email: owner.respond_to?(:email) ? owner.email : nil }) when User data.merge!({ diff --git a/app/views/admin/groups/index.html.haml b/app/views/admin/groups/index.html.haml index 8e45dc76ec6..c9d7c24f204 100644 --- a/app/views/admin/groups/index.html.haml +++ b/app/views/admin/groups/index.html.haml @@ -31,11 +31,8 @@ .clearfix.light.append-bottom-10 %span - %b Owner: - - if group.owner - = link_to group.owner_name, admin_user_path(group.owner) - - else - (deleted) + %b Members: + %span.badge= group.members.size \| %span %b Projects: diff --git a/app/views/admin/groups/show.html.haml b/app/views/admin/groups/show.html.haml index 5509811bae5..1566c345809 100644 --- a/app/views/admin/groups/show.html.haml +++ b/app/views/admin/groups/show.html.haml @@ -25,26 +25,6 @@ = @group.description %li - %span.light Owned by: - %strong - - if @group.owner - = link_to @group.owner_name, admin_user_path(@group.owner) - - else - (deleted) - .pull-right - = link_to "#", class: "btn btn-small change-owner-link" do - %i.icon-edit - Change owner - %li.change-owner-holder.hide.bgred - .form-holder - %strong.cred New Owner: - = form_for [:admin, @group] do |f| - = users_select_tag(:"group[owner_id]") - .prepend-top-10 - = f.submit 'Change Owner', class: "btn btn-remove" - = link_to "Cancel", "#", class: "btn change-owner-cancel-link" - - %li %span.light Created at: %strong = @group.created_at.stamp("March 1, 1999") @@ -92,4 +72,5 @@ = link_to user.name, admin_user_path(user) %span.pull-right.light = member.human_access - + = link_to group_users_group_path(@group, member), confirm: remove_user_from_group_message(@group, user), method: :delete, remote: true, class: "btn-tiny btn btn-remove", title: 'Remove user from group' do + %i.icon-minus.icon-white diff --git a/app/views/admin/projects/show.html.haml b/app/views/admin/projects/show.html.haml index cecc84fe093..c8a87328207 100644 --- a/app/views/admin/projects/show.html.haml +++ b/app/views/admin/projects/show.html.haml @@ -25,7 +25,7 @@ %span.light Owned by: %strong - if @project.owner - = link_to @project.owner_name, admin_user_path(@project.owner) + = link_to @project.owner_name, [:admin, @project.owner] - else (deleted) diff --git a/app/views/groups/edit.html.haml b/app/views/groups/edit.html.haml index 8aefdf7f513..2682d31ff47 100644 --- a/app/views/groups/edit.html.haml +++ b/app/views/groups/edit.html.haml @@ -10,8 +10,6 @@ %i.icon-folder-close Projects %li - = link_to 'Transfer', '#tab-transfer', 'data-toggle' => 'tab' - %li = link_to 'Remove', '#tab-remove', 'data-toggle' => 'tab' .span10 @@ -65,17 +63,6 @@ - if @group.projects.blank? %p.nothing_here_message This group has no projects yet - .tab-pane#tab-transfer - .ui-box.ui-box-danger - .title Transfer group - .ui-box-body - %p - Transferring group will cause loss of admin control over group and all child projects - = form_for @group do |f| - = users_select_tag(:'group[owner_id]') - %hr - = f.submit 'Transfer group', class: "btn btn-small btn-remove" - .tab-pane#tab-remove .ui-box.ui-box-danger .title Remove group diff --git a/app/views/groups/members.html.haml b/app/views/groups/members.html.haml index 77bab11b331..124560e4786 100644 --- a/app/views/groups/members.html.haml +++ b/app/views/groups/members.html.haml @@ -2,6 +2,9 @@ Group members %p.light Members of group have access to all group projects. + Read more about permissions + %strong= link_to "here", help_permissions_path, class: "vlink" + %hr - can_manage_group = current_user.can? :manage_group, @group .ui-box diff --git a/app/views/help/permissions.html.haml b/app/views/help/permissions.html.haml index da5210bad3d..bab1e7c0a41 100644 --- a/app/views/help/permissions.html.haml +++ b/app/views/help/permissions.html.haml @@ -1,6 +1,9 @@ = render layout: 'help/layout' do %h3.page-title Permissions + %p.light User has different abilities depends on access level he has in particular group or project + %hr + %h4 Project: %table.table %thead %tr @@ -158,3 +161,50 @@ %td %td %td.permission-x ✓ + + %h4 Group + %table.table + %thead + %tr + %th Action + %th Guest + %th Reporter + %th Developer + %th Master + %th Owner + %tbody + %tr + %td Browse group + %td.permission-x ✓ + %td.permission-x ✓ + %td.permission-x ✓ + %td.permission-x ✓ + %td.permission-x ✓ + %tr + %td Edit group + %td + %td + %td + %td + %td.permission-x ✓ + %tr + %td Create project in group + %td + %td + %td + %td + %td.permission-x ✓ + %tr + %td Manage group members + %td + %td + %td + %td + %td.permission-x ✓ + %tr + %td Remove group + %td + %td + %td + %td + %td.permission-x ✓ diff --git a/app/views/users_groups/_users_group.html.haml b/app/views/users_groups/_users_group.html.haml index c8d306838f7..5cdb5bb8c40 100644 --- a/app/views/users_groups/_users_group.html.haml +++ b/app/views/users_groups/_users_group.html.haml @@ -10,7 +10,7 @@ %span.pull-right %strong= member.human_access - - if show_controls && user != @group.owner && user != current_user + - if show_controls && can?(current_user, :manage_group, @group) && current_user != user = link_to '#', class: "btn-tiny btn js-toggle-button", title: 'Edit access level' do %i.icon-edit = link_to group_users_group_path(@group, member), confirm: remove_user_from_group_message(@group, user), method: :delete, remote: true, class: "btn-tiny btn btn-remove", title: 'Remove user from group' do @@ -20,4 +20,4 @@ = form_for [@group, member], remote: true do |f| .alert.prepend-top-20 = f.select :group_access, options_for_select(UsersGroup.group_access_roles, member.group_access) - = f.submit 'Save', class: 'btn btn-save' + = f.submit 'Save', class: 'btn btn-save btn-small' diff --git a/db/migrate/20130926081215_change_owner_id_for_group.rb b/db/migrate/20130926081215_change_owner_id_for_group.rb new file mode 100644 index 00000000000..8f1992c37ab --- /dev/null +++ b/db/migrate/20130926081215_change_owner_id_for_group.rb @@ -0,0 +1,9 @@ +class ChangeOwnerIdForGroup < ActiveRecord::Migration + def up + change_column :namespaces, :owner_id, :integer, null: true + end + + def down + change_column :namespaces, :owner_id, :integer, null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index d5effe40ea1..713d9f733d6 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended to check this file into your version control system. -ActiveRecord::Schema.define(:version => 20130909132950) do +ActiveRecord::Schema.define(:version => 20130926081215) do create_table "deploy_keys_projects", :force => true do |t| t.integer "deploy_key_id", :null => false @@ -129,7 +129,7 @@ ActiveRecord::Schema.define(:version => 20130909132950) do create_table "namespaces", :force => true do |t| t.string "name", :null => false t.string "path", :null => false - t.integer "owner_id", :null => false + t.integer "owner_id" t.datetime "created_at", :null => false t.datetime "updated_at", :null => false t.string "type" diff --git a/features/steps/group/group.rb b/features/steps/group/group.rb index 71f29121179..99ec77a7613 100644 --- a/features/steps/group/group.rb +++ b/features/steps/group/group.rb @@ -10,7 +10,8 @@ class Groups < Spinach::FeatureSteps end And 'I have group with projects' do - @group = create(:group, owner: current_user) + @group = create(:group) + @group.add_owner(current_user) @project = create(:project, namespace: @group) @event = create(:closed_issue_event, project: @project) diff --git a/spec/contexts/projects_create_context_spec.rb b/spec/contexts/projects_create_context_spec.rb index 7b5fa530a8a..8b2a49dbee5 100644 --- a/spec/contexts/projects_create_context_spec.rb +++ b/spec/contexts/projects_create_context_spec.rb @@ -25,13 +25,15 @@ describe Projects::CreateContext do context 'group namespace' do before do - @group = create :group, owner: @user + @group = create :group + @group.add_owner(@user) + @opts.merge!(namespace_id: @group.id) @project = create_project(@user, @opts) end it { @project.should be_valid } - it { @project.owner.should == @user } + it { @project.owner.should == @group } it { @project.namespace.should == @group } end diff --git a/spec/factories.rb b/spec/factories.rb index 4c5e687ac3e..56561fe4595 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -71,7 +71,6 @@ FactoryGirl.define do factory :group do sequence(:name) { |n| "group#{n}" } path { name.downcase.gsub(/\s/, '_') } - owner type 'Group' end diff --git a/spec/features/security/group_access_spec.rb b/spec/features/security/group_access_spec.rb index b6167174f20..dea957962a8 100644 --- a/spec/features/security/group_access_spec.rb +++ b/spec/features/security/group_access_spec.rb @@ -10,11 +10,13 @@ describe "Group access" do describe "Group" do let(:group) { create(:group) } + let(:owner) { create(:owner) } let(:master) { create(:user) } let(:reporter) { create(:user) } let(:guest) { create(:user) } before do + group.add_user(owner, Gitlab::Access::OWNER) group.add_user(master, Gitlab::Access::MASTER) group.add_user(reporter, Gitlab::Access::REPORTER) group.add_user(guest, Gitlab::Access::GUEST) @@ -23,7 +25,7 @@ describe "Group access" do describe "GET /groups/:path" do subject { group_path(group) } - it { should be_allowed_for group.owner } + it { should be_allowed_for owner } it { should be_allowed_for master } it { should be_allowed_for reporter } it { should be_allowed_for :admin } @@ -35,7 +37,7 @@ describe "Group access" do describe "GET /groups/:path/issues" do subject { issues_group_path(group) } - it { should be_allowed_for group.owner } + it { should be_allowed_for owner } it { should be_allowed_for master } it { should be_allowed_for reporter } it { should be_allowed_for :admin } @@ -47,7 +49,7 @@ describe "Group access" do describe "GET /groups/:path/merge_requests" do subject { merge_requests_group_path(group) } - it { should be_allowed_for group.owner } + it { should be_allowed_for owner } it { should be_allowed_for master } it { should be_allowed_for reporter } it { should be_allowed_for :admin } @@ -59,7 +61,7 @@ describe "Group access" do describe "GET /groups/:path/members" do subject { members_group_path(group) } - it { should be_allowed_for group.owner } + it { should be_allowed_for owner } it { should be_allowed_for master } it { should be_allowed_for reporter } it { should be_allowed_for :admin } @@ -71,7 +73,7 @@ describe "Group access" do describe "GET /groups/:path/edit" do subject { edit_group_path(group) } - it { should be_allowed_for group.owner } + it { should be_allowed_for owner } it { should be_denied_for master } it { should be_denied_for reporter } it { should be_allowed_for :admin } diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 78c39548eb4..ce1aa05bcd7 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -26,10 +26,10 @@ describe Group do it { should validate_uniqueness_of(:name) } it { should validate_presence_of :path } it { should validate_uniqueness_of(:path) } - it { should validate_presence_of :owner } + it { should_not validate_presence_of :owner } describe :users do - it { group.users.should == [group.owner] } + it { group.users.should == group.owners } end describe :human_name do @@ -38,7 +38,7 @@ describe Group do describe :add_users do let(:user) { create(:user) } - before { group.add_users([user.id], UsersGroup::MASTER) } + before { group.add_user(user, UsersGroup::MASTER) } it { group.users_groups.masters.map(&:user).should include(user) } end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index c3d2bf5d4a6..47ae760a7ed 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -85,7 +85,6 @@ describe Project do it { should respond_to(:execute_hooks) } it { should respond_to(:transfer) } it { should respond_to(:name_with_namespace) } - it { should respond_to(:namespace_owner) } it { should respond_to(:owner) } it { should respond_to(:path_with_namespace) } end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index c879900f8fd..f6c9f82c4ee 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -130,11 +130,12 @@ describe User do before do ActiveRecord::Base.observers.enable(:user_observer) @user = create :user - @group = create :group, owner: @user + @group = create :group + @group.add_owner(@user) end it { @user.several_namespaces?.should be_true } - it { @user.namespaces.should include(@user.namespace, @group) } + it { @user.namespaces.should include(@user.namespace) } it { @user.authorized_groups.should == [@group] } it { @user.owned_groups.should == [@group] } end @@ -144,9 +145,10 @@ describe User do ActiveRecord::Base.observers.enable(:user_observer) @user = create :user @user2 = create :user - @group = create :group, owner: @user + @group = create :group + @group.add_owner(@user) - @group.add_users([@user2.id], UsersGroup::OWNER) + @group.add_user(@user2, UsersGroup::OWNER) end it { @user2.several_namespaces?.should be_true } diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index f7fd27523b0..a6ce72e11e9 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -6,8 +6,13 @@ describe API::API do let(:user1) { create(:user) } let(:user2) { create(:user) } let(:admin) { create(:admin) } - let!(:group1) { create(:group, owner: user1) } - let!(:group2) { create(:group, owner: user2) } + let!(:group1) { create(:group) } + let!(:group2) { create(:group) } + + before do + group1.add_owner(user1) + group2.add_owner(user2) + end describe "GET /groups" do context "when unauthenticated" do @@ -130,14 +135,19 @@ describe API::API do let(:master) { create(:user) } let(:guest) { create(:user) } let!(:group_with_members) do - group = create(:group, owner: owner) + group = create(:group) group.add_users([reporter.id], UsersGroup::REPORTER) group.add_users([developer.id], UsersGroup::DEVELOPER) group.add_users([master.id], UsersGroup::MASTER) group.add_users([guest.id], UsersGroup::GUEST) group end - let!(:group_no_members) { create(:group, owner: owner) } + let!(:group_no_members) { create(:group) } + + before do + group_with_members.add_owner owner + group_no_members.add_owner owner + end describe "GET /groups/:id/members" do context "when authenticated as user that is part or the group" do |