Merge branch 'dz-refactor-create-members' into 'master'

Refactor code that creates and destroys project/group members

See merge request !10735

13 files changed, 200 insertions, 248 deletions

diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb
index c13333641d3..b1bacc8ffe5 100644
--- a/app/controllers/concerns/membership_actions.rb
+++ b/app/controllers/concerns/membership_actions.rb
@@ -1,6 +1,32 @@
 module MembershipActions
   extend ActiveSupport::Concern
 
+  def create
+    status = Members::CreateService.new(membershipable, current_user, params).execute
+
+    redirect_url = members_page_url
+
+    if status
+      redirect_to redirect_url, notice: 'Users were successfully added.'
+    else
+      redirect_to redirect_url, alert: 'No users specified.'
+    end
+  end
+
+  def destroy
+    Members::DestroyService.new(membershipable, current_user, params).
+      execute(:all)
+
+    respond_to do |format|
+      format.html do
+        message = "User was successfully removed from #{source_type}."
+        redirect_to members_page_url, notice: message
+      end
+
+      format.js { head :ok }
+    end
+  end
+
   def request_access
     membershipable.request_access(current_user)
 
@@ -11,20 +37,20 @@ module MembershipActions
   def approve_access_request
     Members::ApproveAccessRequestService.new(membershipable, current_user, params).execute
 
-    redirect_to polymorphic_url([membershipable, :members])
+    redirect_to members_page_url
   end
 
   def leave
     member = Members::DestroyService.new(membershipable, current_user, user_id: current_user.id).
       execute(:all)
 
-    source_type = membershipable.class.to_s.humanize(capitalize: false)
     notice =
       if member.request?
         "Your access request to the #{source_type} has been withdrawn."
       else
         "You left the \"#{membershipable.human_name}\" #{source_type}."
       end
+
     redirect_path = member.request? ? member.source : [:dashboard, membershipable.class.to_s.tableize]
 
     redirect_to redirect_path, notice: notice
@@ -35,4 +61,16 @@ module MembershipActions
   def membershipable
     raise NotImplementedError
   end
+
+  def members_page_url
+    if membershipable.is_a?(Project)
+      project_settings_members_path(membershipable)
+    else
+      polymorphic_url([membershipable, :members])
+    end
+  end
+
+  def source_type
+    @source_type ||= membershipable.class.to_s.humanize(capitalize: false)
+  end
 end
diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb
index 00c50f9d0ad..8fc234a62b1 100644
--- a/app/controllers/groups/group_members_controller.rb
+++ b/app/controllers/groups/group_members_controller.rb
@@ -21,21 +21,6 @@ class Groups::GroupMembersController < Groups::ApplicationController
     @group_member = @group.group_members.new
   end
 
-  def create
-    if params[:user_ids].blank?
-      return redirect_to(group_group_members_path(@group), alert: 'No users specified.')
-    end
-
-    @group.add_users(
-      params[:user_ids].split(','),
-      params[:access_level],
-      current_user: current_user,
-      expires_at: params[:expires_at]
-    )
-
-    redirect_to group_group_members_path(@group), notice: 'Users were successfully added.'
-  end
-
   def update
     @group_member = @group.group_members.find(params[:id])
 
@@ -44,15 +29,6 @@ class Groups::GroupMembersController < Groups::ApplicationController
     @group_member.update_attributes(member_params)
   end
 
-  def destroy
-    Members::DestroyService.new(@group, current_user, id: params[:id]).execute(:all)
-
-    respond_to do |format|
-      format.html { redirect_to group_group_members_path(@group), notice: 'User was successfully removed from group.' }
-      format.js { head :ok }
-    end
-  end
-
   def resend_invite
     redirect_path = group_group_members_path(@group)
 
diff --git a/app/controllers/projects/project_members_controller.rb b/app/controllers/projects/project_members_controller.rb
index 6e158e685e9..d2d26738582 100644
--- a/app/controllers/projects/project_members_controller.rb
+++ b/app/controllers/projects/project_members_controller.rb
@@ -10,18 +10,6 @@ class Projects::ProjectMembersController < Projects::ApplicationController
     redirect_to namespace_project_settings_members_path(@project.namespace, @project, sort: sort)
   end
 
-  def create
-    status = Members::CreateService.new(@project, current_user, params).execute
-
-    redirect_url = namespace_project_settings_members_path(@project.namespace, @project)
-
-    if status
-      redirect_to redirect_url, notice: 'Users were successfully added.'
-    else
-      redirect_to redirect_url, alert: 'No users or groups specified.'
-    end
-  end
-
   def update
     @project_member = @project.project_members.find(params[:id])
 
@@ -30,18 +18,6 @@ class Projects::ProjectMembersController < Projects::ApplicationController
     @project_member.update_attributes(member_params)
   end
 
-  def destroy
-    Members::DestroyService.new(@project, current_user, params).
-      execute(:all)
-
-    respond_to do |format|
-      format.html do
-        redirect_to namespace_project_settings_members_path(@project.namespace, @project)
-      end
-      format.js { head :ok }
-    end
-  end
-
   def resend_invite
     redirect_path = namespace_project_settings_members_path(@project.namespace, @project)
 
diff --git a/app/models/project.rb b/app/models/project.rb
index a160efba912..73593f04283 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -181,7 +181,7 @@ class Project < ActiveRecord::Base
   delegate :name, to: :owner, allow_nil: true, prefix: true
   delegate :count, to: :forks, prefix: true
   delegate :members, to: :team, prefix: true
-  delegate :add_user, to: :team
+  delegate :add_user, :add_users, to: :team
   delegate :add_guest, :add_reporter, :add_developer, :add_master, to: :team
   delegate :empty_repo?, to: :repository
 
diff --git a/app/services/members/create_service.rb b/app/services/members/create_service.rb
index e4b24ccef92..3a58f6c065d 100644
--- a/app/services/members/create_service.rb
+++ b/app/services/members/create_service.rb
@@ -1,9 +1,15 @@
 module Members
   class CreateService < BaseService
+    def initialize(source, current_user, params = {})
+      @source = source
+      @current_user = current_user
+      @params = params
+    end
+
     def execute
       return false if params[:user_ids].blank?
 
-      project.team.add_users(
+      @source.add_users(
         params[:user_ids].split(','),
         params[:access_level],
         expires_at: params[:expires_at],
diff --git a/changelogs/unreleased/dz-refactor-create-members.yml b/changelogs/unreleased/dz-refactor-create-members.yml
new file mode 100644
index 00000000000..8cff21eabb1
--- /dev/null
+++ b/changelogs/unreleased/dz-refactor-create-members.yml
@@ -0,0 +1,4 @@
+---
+title: Refactor code that creates project/group members
+merge_request: 10735
+author:
diff --git a/features/group/members.feature b/features/group/members.feature
index 1f9514bac39..e539f6a1273 100644
--- a/features/group/members.feature
+++ b/features/group/members.feature
@@ -4,40 +4,6 @@ Feature: Group Members
     And "John Doe" is owner of group "Owned"
     And "John Doe" is guest of group "Guest"
 
-  @javascript
-  Scenario: I should add user to group "Owned"
-    Given User "Mary Jane" exists
-    When I visit group "Owned" members page
-    And I select user "Mary Jane" from list with role "Reporter"
-    Then I should see user "Mary Jane" in team list
-
-  @javascript
-  Scenario: Add user to group
-    Given gitlab user "Mike"
-    When I visit group "Owned" members page
-    When I select "Mike" as "Reporter"
-    Then I should see "Mike" in team list as "Reporter"
-
-  @javascript
-  Scenario: Ignore add user to group when is already Owner
-    Given gitlab user "Mike"
-    When I visit group "Owned" members page
-    When I select "Mike" as "Reporter"
-    Then I should see "Mike" in team list as "Owner"
-
-  @javascript
-  Scenario: Invite user to group
-    When I visit group "Owned" members page
-    When I select "sjobs@apple.com" as "Reporter"
-    Then I should see "sjobs@apple.com" in team list as invited "Reporter"
-
-  @javascript
-  Scenario: Edit group member permissions
-    Given "Mary Jane" is guest of group "Owned"
-    And I visit group "Owned" members page
-    When I change the "Mary Jane" role to "Developer"
-    Then I should see "Mary Jane" as "Developer"
-
   # Leave
 
   @javascript
diff --git a/features/project/team_management.feature b/features/project/team_management.feature
index 5888662fc3f..aed41924cd9 100644
--- a/features/project/team_management.feature
+++ b/features/project/team_management.feature
@@ -7,26 +7,6 @@ Feature: Project Team Management
     And "Dmitriy" is "Shop" developer
     And I visit project "Shop" team page
 
-  Scenario: See all team members
-    Then I should be able to see myself in team
-    And I should see "Dmitriy" in team list
-
-  @javascript
-  Scenario: Add user to project
-    When I select "Mike" as "Reporter"
-    Then I should see "Mike" in team list as "Reporter"
-
-  @javascript
-  Scenario: Invite user to project
-    When I select "sjobs@apple.com" as "Reporter"
-    Then I should see "sjobs@apple.com" in team list as invited "Reporter"
-
-  @javascript
-  Scenario: Update user access
-    Given I should see "Dmitriy" in team list as "Developer"
-    And I change "Dmitriy" role to "Reporter"
-    And I should see "Dmitriy" in team list as "Reporter"
-
   Scenario: Cancel team member
     Given I click cancel link for "Dmitriy"
     Then I visit project "Shop" team page
diff --git a/features/steps/group/members.rb b/features/steps/group/members.rb
index dcbf36df656..b04a7015d4e 100644
--- a/features/steps/group/members.rb
+++ b/features/steps/group/members.rb
@@ -4,71 +4,6 @@ class Spinach::Features::GroupMembers < Spinach::FeatureSteps
   include SharedPaths
   include SharedGroup
   include SharedUser
-  include Select2Helper
-
-  step 'I select "Mike" as "Reporter"' do
-    user = User.find_by(name: "Mike")
-
-    page.within ".users-group-form" do
-      select2(user.id, from: "#user_ids", multiple: true)
-      select "Reporter", from: "access_level"
-    end
-
-    click_button "Add to group"
-  end
-
-  step 'I select "Mike" as "Master"' do
-    user = User.find_by(name: "Mike")
-
-    page.within ".users-group-form" do
-      select2(user.id, from: "#user_ids", multiple: true)
-      select "Master", from: "access_level"
-    end
-
-    click_button "Add to group"
-  end
-
-  step 'I should see "Mike" in team list as "Reporter"' do
-    page.within '.content-list' do
-      expect(page).to have_content('Mike')
-      expect(page).to have_content('Reporter')
-    end
-  end
-
-  step 'I should see "Mike" in team list as "Owner"' do
-    page.within '.content-list' do
-      expect(page).to have_content('Mike')
-      expect(page).to have_content('Owner')
-    end
-  end
-
-  step 'I select "sjobs@apple.com" as "Reporter"' do
-    page.within ".users-group-form" do
-      select2("sjobs@apple.com", from: "#user_ids", multiple: true)
-      select "Reporter", from: "access_level"
-    end
-
-    click_button "Add to group"
-  end
-
-  step 'I should see "sjobs@apple.com" in team list as invited "Reporter"' do
-    page.within '.content-list' do
-      expect(page).to have_content('sjobs@apple.com')
-      expect(page).to have_content('Invited')
-      expect(page).to have_content('Reporter')
-    end
-  end
-
-  step 'I select user "Mary Jane" from list with role "Reporter"' do
-    user = User.find_by(name: "Mary Jane") || create(:user, name: "Mary Jane")
-
-    page.within ".users-group-form" do
-      select2(user.id, from: "#user_ids", multiple: true)
-      select "Reporter", from: "access_level"
-    end
-
-    click_button "Add to group"
-  end
 
   step 'I should see user "John Doe" in team list' do
     expect(group_members_list).to have_content("John Doe")
diff --git a/features/steps/project/team_management.rb b/features/steps/project/team_management.rb
index 6986c7ede56..ff4c9deee2a 100644
--- a/features/steps/project/team_management.rb
+++ b/features/steps/project/team_management.rb
@@ -4,25 +4,10 @@ class Spinach::Features::ProjectTeamManagement < Spinach::FeatureSteps
   include SharedPaths
   include Select2Helper
 
-  step 'I should be able to see myself in team' do
-    expect(page).to have_content(@user.name)
-    expect(page).to have_content(@user.username)
-  end
-
-  step 'I should see "Dmitriy" in team list' do
+  step 'I should not see "Dmitriy" in team list' do
     user = User.find_by(name: "Dmitriy")
-    expect(page).to have_content(user.name)
-    expect(page).to have_content(user.username)
-  end
-
-  step 'I select "Mike" as "Reporter"' do
-    user = User.find_by(name: "Mike")
-
-    page.within ".users-project-form" do
-      select2(user.id, from: "#user_ids", multiple: true)
-      select "Reporter", from: "access_level"
-    end
-    click_button "Add to project"
+    expect(page).not_to have_content(user.name)
+    expect(page).not_to have_content(user.username)
   end
 
   step 'I should see "Mike" in team list as "Reporter"' do
@@ -34,60 +19,6 @@ class Spinach::Features::ProjectTeamManagement < Spinach::FeatureSteps
     end
   end
 
-  step 'I select "sjobs@apple.com" as "Reporter"' do
-    page.within ".users-project-form" do
-      find('#user_ids', visible: false).set('sjobs@apple.com')
-      select "Reporter", from: "access_level"
-    end
-    click_button "Add to project"
-  end
-
-  step 'I should see "sjobs@apple.com" in team list as invited "Reporter"' do
-    project_member = project.project_members.find_by(invite_email: 'sjobs@apple.com')
-    page.within "#project_member_#{project_member.id}" do
-      expect(page).to have_content('sjobs@apple.com')
-      expect(page).to have_content('Invited')
-      expect(page).to have_content('Reporter')
-    end
-  end
-
-  step 'I should see "Dmitriy" in team list as "Developer"' do
-    user = User.find_by(name: 'Dmitriy')
-    project_member = project.project_members.find_by(user_id: user.id)
-    page.within "#project_member_#{project_member.id}" do
-      expect(page).to have_content('Dmitriy')
-      expect(page).to have_content('Developer')
-    end
-  end
-
-  step 'I change "Dmitriy" role to "Reporter"' do
-    project = Project.find_by(name: "Shop")
-    user = User.find_by(name: 'Dmitriy')
-    project_member = project.project_members.find_by(user_id: user.id)
-    page.within "#project_member_#{project_member.id}" do
-      click_button project_member.human_access
-
-      page.within '.dropdown-menu' do
-        click_link 'Reporter'
-      end
-    end
-  end
-
-  step 'I should see "Dmitriy" in team list as "Reporter"' do
-    user = User.find_by(name: 'Dmitriy')
-    project_member = project.project_members.find_by(user_id: user.id)
-    page.within "#project_member_#{project_member.id}" do
-      expect(page).to have_content('Dmitriy')
-      expect(page).to have_content('Reporter')
-    end
-  end
-
-  step 'I should not see "Dmitriy" in team list' do
-    user = User.find_by(name: "Dmitriy")
-    expect(page).not_to have_content(user.name)
-    expect(page).not_to have_content(user.username)
-  end
-
   step 'gitlab user "Mike"' do
     create(:user, name: "Mike")
   end
@@ -113,7 +44,7 @@ class Spinach::Features::ProjectTeamManagement < Spinach::FeatureSteps
     project.team << [user, :reporter]
   end
 
-  step 'I click link "Import team from another project"' do    
+  step 'I click link "Import team from another project"' do
     page.within '.users-project-form' do
       click_link "Import"
     end
diff --git a/spec/controllers/projects/project_members_controller_spec.rb b/spec/controllers/projects/project_members_controller_spec.rb
index 416eaa0037e..a4b4392d7cc 100644
--- a/spec/controllers/projects/project_members_controller_spec.rb
+++ b/spec/controllers/projects/project_members_controller_spec.rb
@@ -55,7 +55,7 @@ describe Projects::ProjectMembersController do
                       user_ids: '',
                       access_level: Gitlab::Access::GUEST
 
-        expect(response).to set_flash.to 'No users or groups specified.'
+        expect(response).to set_flash.to 'No users specified.'
         expect(response).to redirect_to(namespace_project_settings_members_path(project.namespace, project))
       end
     end
@@ -225,7 +225,7 @@ describe Projects::ProjectMembersController do
                                         id: member
 
           expect(response).to redirect_to(
-            namespace_project_project_members_path(project.namespace, project)
+            namespace_project_settings_members_path(project.namespace, project)
           )
           expect(project.members).to include member
         end
diff --git a/spec/features/groups/members/list_spec.rb b/spec/features/groups/members/list_spec.rb
index 14c193f7450..543879bd21d 100644
--- a/spec/features/groups/members/list_spec.rb
+++ b/spec/features/groups/members/list_spec.rb
@@ -1,6 +1,8 @@
 require 'spec_helper'
 
 feature 'Groups members list', feature: true do
+  include Select2Helper
+
   let(:user1) { create(:user, name: 'John Doe') }
   let(:user2) { create(:user, name: 'Mary Jane') }
   let(:group) { create(:group) }
@@ -30,7 +32,7 @@ feature 'Groups members list', feature: true do
     expect(second_row).to be_blank
   end
 
-  it 'updates user to owner level', :js do
+  scenario 'update user to owner level', :js do
     group.add_owner(user1)
     group.add_developer(user2)
 
@@ -38,13 +40,52 @@ feature 'Groups members list', feature: true do
 
     page.within(second_row) do
       click_button('Developer')
-
       click_link('Owner')
 
       expect(page).to have_button('Owner')
     end
   end
 
+  scenario 'add user to group', :js do
+    group.add_owner(user1)
+
+    visit group_group_members_path(group)
+
+    add_user(user2.id, 'Reporter')
+
+    page.within(second_row) do
+      expect(page).to have_content(user2.name)
+      expect(page).to have_button('Reporter')
+    end
+  end
+
+  scenario 'add yourself to group when already an owner', :js do
+    group.add_owner(user1)
+
+    visit group_group_members_path(group)
+
+    add_user(user1.id, 'Reporter')
+
+    page.within(first_row) do
+      expect(page).to have_content(user1.name)
+      expect(page).to have_content('Owner')
+    end
+  end
+
+  scenario 'invite user to group', :js do
+    group.add_owner(user1)
+
+    visit group_group_members_path(group)
+
+    add_user('test@example.com', 'Reporter')
+
+    page.within(second_row) do
+      expect(page).to have_content('test@example.com')
+      expect(page).to have_content('Invited')
+      expect(page).to have_button('Reporter')
+    end
+  end
+
   def first_row
     page.all('ul.content-list > li')[0]
   end
@@ -52,4 +93,13 @@ feature 'Groups members list', feature: true do
   def second_row
     page.all('ul.content-list > li')[1]
   end
+
+  def add_user(id, role)
+    page.within ".users-group-form" do
+      select2(id, from: "#user_ids", multiple: true)
+      select(role, from: "access_level")
+    end
+
+    click_button "Add to group"
+  end
 end
diff --git a/spec/features/projects/members/list_spec.rb b/spec/features/projects/members/list_spec.rb
new file mode 100644
index 00000000000..deea34214fb
--- /dev/null
+++ b/spec/features/projects/members/list_spec.rb
@@ -0,0 +1,90 @@
+require 'spec_helper'
+
+feature 'Project members list', feature: true do
+  include Select2Helper
+
+  let(:user1) { create(:user, name: 'John Doe') }
+  let(:user2) { create(:user, name: 'Mary Jane') }
+  let(:group) { create(:group) }
+  let(:project) { create(:project, namespace: group) }
+
+  background do
+    login_as(user1)
+    group.add_owner(user1)
+  end
+
+  scenario 'show members from project and group' do
+    project.add_developer(user2)
+
+    visit_members_page
+
+    expect(first_row.text).to include(user1.name)
+    expect(second_row.text).to include(user2.name)
+  end
+
+  scenario 'show user once if member of both group and project' do
+    project.add_developer(user1)
+
+    visit_members_page
+
+    expect(first_row.text).to include(user1.name)
+    expect(second_row).to be_blank
+  end
+
+  scenario 'update user acess level', :js do
+    project.add_developer(user2)
+
+    visit_members_page
+
+    page.within(second_row) do
+      click_button('Developer')
+      click_link('Reporter')
+
+      expect(page).to have_button('Reporter')
+    end
+  end
+
+  scenario 'add user to project', :js do
+    visit_members_page
+
+    add_user(user2.id, 'Reporter')
+
+    page.within(second_row) do
+      expect(page).to have_content(user2.name)
+      expect(page).to have_button('Reporter')
+    end
+  end
+
+  scenario 'invite user to project', :js do
+    visit_members_page
+
+    add_user('test@example.com', 'Reporter')
+
+    page.within(second_row) do
+      expect(page).to have_content('test@example.com')
+      expect(page).to have_content('Invited')
+      expect(page).to have_button('Reporter')
+    end
+  end
+
+  def first_row
+    page.all('ul.content-list > li')[0]
+  end
+
+  def second_row
+    page.all('ul.content-list > li')[1]
+  end
+
+  def add_user(id, role)
+    page.within ".users-project-form" do
+      select2(id, from: "#user_ids", multiple: true)
+      select(role, from: "access_level")
+    end
+
+    click_button "Add to project"
+  end
+
+  def visit_members_page
+    visit namespace_project_settings_members_path(project.namespace, project)
+  end
+end