Merge branch 'refactor/group_owners' of /home/git/repositories/gitlab/gitlabhq

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"

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"

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.")

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.' }

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

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
-
-    if membership
-      membership.update_attributes(group_access: UsersGroup::OWNER)
-    else
-      add_owner
+  def add_owner(user)
+    self.add_user(user, UsersGroup::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

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,

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

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

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!({

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:

app/views/admin/groups/show.html.haml

@@ -24,26 +24,6 @@
           %strong
             = @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
@@ -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

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)
 

app/views/groups/edit.html.haml

@@ -9,8 +9,6 @@
         = link_to '#tab-projects', 'data-toggle' => 'tab' do
           %i.icon-folder-close
           Projects
-      %li
-        = link_to 'Transfer', '#tab-transfer', 'data-toggle' => 'tab'
       %li
         = link_to 'Remove', '#tab-remove', 'data-toggle' => 'tab'
 
@@ -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

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

app/views/help/permissions.html.haml

 = 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 ✓

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'

db/migrate/20130926081215_change_owner_id_for_group.rb

+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

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"

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)
 

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
 

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
 

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 }

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

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

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 }

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