Merge branch 'yesbabyyes/gitlab-ce-22967-adding-zero-users-to-group-reports-success'

Merge request - https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6979

11 files changed, 124 insertions, 30 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index ca6b9bc99d5..02291b1679a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -16,6 +16,8 @@ Please view this file on the master branch, on stable branches it's out of date.
   - Fix reply-by-email not working due to queue name mismatch
   - Expire and build repository cache after project import
   - Fix 404 for group pages when GitLab setup uses relative url
+  - Simpler arguments passed to named_route on toggle_award_url helper method 
+  - Better handle when no users were selected for adding to group or project. (Linus Thiel)
 
 ## 8.13.0 (2016-10-22)
   - Removes extra line for empty issue description. (!7045)
diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js
index 8a61669822c..17cbfd0e66f 100644
--- a/app/assets/javascripts/application.js
+++ b/app/assets/javascripts/application.js
@@ -83,14 +83,15 @@
   };
 
   // Disable button if text field is empty
-  window.disableButtonIfEmptyField = function(field_selector, button_selector) {
+  window.disableButtonIfEmptyField = function(field_selector, button_selector, event_name) {
+    event_name = event_name || 'input';
     var closest_submit, field;
     field = $(field_selector);
     closest_submit = field.closest('form').find(button_selector);
     if (rstrip(field.val()) === "") {
       closest_submit.disable();
     }
-    return field.on('input', function() {
+    return field.on(event_name, function() {
       if (rstrip($(this).val()) === "") {
         return closest_submit.disable();
       } else {
diff --git a/app/assets/javascripts/members.js.es6 b/app/assets/javascripts/members.js.es6
index a0cd20f21e8..2bdd0f7a637 100644
--- a/app/assets/javascripts/members.js.es6
+++ b/app/assets/javascripts/members.js.es6
@@ -10,6 +10,7 @@
       $('.project_member, .group_member').off('ajax:success').on('ajax:success', this.removeRow);
       $('.js-member-update-control').off('change').on('change', this.formSubmit);
       $('.js-edit-member-form').off('ajax:success').on('ajax:success', this.formSuccess);
+      disableButtonIfEmptyField('#user_ids', 'input[name=commit]', 'change');
     }
 
     removeRow(e) {
diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb
index 18cd800c619..940a3ad20ba 100644
--- a/app/controllers/groups/group_members_controller.rb
+++ b/app/controllers/groups/group_members_controller.rb
@@ -21,6 +21,10 @@ class Groups::GroupMembersController < Groups::ApplicationController
   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],
diff --git a/app/controllers/projects/project_members_controller.rb b/app/controllers/projects/project_members_controller.rb
index 2a07d154853..d08f490de18 100644
--- a/app/controllers/projects/project_members_controller.rb
+++ b/app/controllers/projects/project_members_controller.rb
@@ -25,6 +25,10 @@ class Projects::ProjectMembersController < Projects::ApplicationController
   end
 
   def create
+    if params[:user_ids].blank?
+      return redirect_to(namespace_project_project_members_path(@project.namespace, @project), alert: 'No users or groups specified.')
+    end
+
     @project.team.add_users(
       params[:user_ids].split(','),
       params[:access_level],
@@ -32,7 +36,7 @@ class Projects::ProjectMembersController < Projects::ApplicationController
       current_user: current_user
     )
 
-    redirect_to namespace_project_project_members_path(@project.namespace, @project)
+    redirect_to namespace_project_project_members_path(@project.namespace, @project), notice: 'Users were successfully added.'
   end
 
   def update
diff --git a/app/models/group.rb b/app/models/group.rb
index 6865e610718..00a595d2705 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -68,7 +68,7 @@ class Group < Namespace
   end
 
   def web_url
-    Gitlab::Routing.url_helpers.group_canonical_url(self)
+    Gitlab::Routing.url_helpers.group_url(self)
   end
 
   def human_name
diff --git a/config/routes/group.rb b/config/routes/group.rb
index 826048ba196..4838c9d91c6 100644
--- a/config/routes/group.rb
+++ b/config/routes/group.rb
@@ -12,26 +12,23 @@ constraints(GroupUrlConstrainer.new) do
   end
 end
 
-scope constraints: { id: /[a-zA-Z.0-9_\-]+(?<!\.atom)/ } do
-  resources :groups, except: [:show] do
-    member do
-      get :issues
-      get :merge_requests
-      get :projects
-      get :activity
-    end
+resources :groups, constraints: { id: /[a-zA-Z.0-9_\-]+(?<!\.atom)/ }  do
+  member do
+    get :issues
+    get :merge_requests
+    get :projects
+    get :activity
+  end
 
-    scope module: :groups do
-      resources :group_members, only: [:index, :create, :update, :destroy], concerns: :access_requestable do
-        post :resend_invite, on: :member
-        delete :leave, on: :collection
-      end
+  scope module: :groups do
+    resources :group_members, only: [:index, :create, :update, :destroy], concerns: :access_requestable do
+      post :resend_invite, on: :member
+      delete :leave, on: :collection
+    end
 
-      resource :avatar, only: [:destroy]
-      resources :milestones, constraints: { id: /[^\/]+/ }, only: [:index, :show, :update, :new, :create]
+    resource :avatar, only: [:destroy]
+    resources :milestones, constraints: { id: /[^\/]+/ }, only: [:index, :show, :update, :new, :create]
 
-      resources :labels, except: [:show], constraints: { id: /\d+/ }
-    end
+    resources :labels, except: [:show], constraints: { id: /\d+/ }
   end
-  get 'groups/:id' => 'groups#show', as: :group_canonical
 end
diff --git a/spec/controllers/groups/group_members_controller_spec.rb b/spec/controllers/groups/group_members_controller_spec.rb
index ad15b3f8f40..c7db84dd5f9 100644
--- a/spec/controllers/groups/group_members_controller_spec.rb
+++ b/spec/controllers/groups/group_members_controller_spec.rb
@@ -13,6 +13,49 @@ describe Groups::GroupMembersController do
     end
   end
 
+  describe 'POST create' do
+    let(:group_user) { create(:user) }
+
+    before { sign_in(user) }
+
+    context 'when user does not have enough rights' do
+      before { group.add_developer(user) }
+
+      it 'returns 403' do
+        post :create, group_id: group,
+                      user_ids: group_user.id,
+                      access_level: Gitlab::Access::GUEST
+
+        expect(response).to have_http_status(403)
+        expect(group.users).not_to include group_user
+      end
+    end
+
+    context 'when user has enough rights' do
+      before { group.add_owner(user) }
+
+      it 'adds user to members' do
+        post :create, group_id: group,
+                      user_ids: group_user.id,
+                      access_level: Gitlab::Access::GUEST
+
+        expect(response).to set_flash.to 'Users were successfully added.'
+        expect(response).to redirect_to(group_group_members_path(group))
+        expect(group.users).to include group_user
+      end
+
+      it 'adds no user to members' do
+        post :create, group_id: group,
+                      user_ids: '',
+                      access_level: Gitlab::Access::GUEST
+
+        expect(response).to set_flash.to 'No users specified.'
+        expect(response).to redirect_to(group_group_members_path(group))
+        expect(group.users).not_to include group_user
+      end
+    end
+  end
+
   describe 'DELETE destroy' do
     let(:member) { create(:group_member, :developer, group: group) }
 
diff --git a/spec/controllers/projects/project_members_controller_spec.rb b/spec/controllers/projects/project_members_controller_spec.rb
index 5e487241d07..b4f066d8600 100644
--- a/spec/controllers/projects/project_members_controller_spec.rb
+++ b/spec/controllers/projects/project_members_controller_spec.rb
@@ -13,6 +13,54 @@ describe Projects::ProjectMembersController do
     end
   end
 
+  describe 'POST create' do
+    context 'when users are added' do
+      let(:project_user) { create(:user) }
+
+      before { sign_in(user) }
+
+      context 'when user does not have enough rights' do
+        before { project.team << [user, :developer] }
+
+        it 'returns 404' do
+          post :create, namespace_id: project.namespace,
+                        project_id: project,
+                        user_ids: project_user.id,
+                        access_level: Gitlab::Access::GUEST
+
+          expect(response).to have_http_status(404)
+          expect(project.users).not_to include project_user
+        end
+      end
+
+      context 'when user has enough rights' do
+        before { project.team << [user, :master] }
+
+        it 'adds user to members' do
+          post :create, namespace_id: project.namespace,
+                        project_id: project,
+                        user_ids: project_user.id,
+                        access_level: Gitlab::Access::GUEST
+
+          expect(response).to set_flash.to 'Users were successfully added.'
+          expect(response).to redirect_to(namespace_project_project_members_path(project.namespace, project))
+          expect(project.users).to include project_user
+        end
+
+        it 'adds no user to members' do
+          post :create, namespace_id: project.namespace,
+                        project_id: project,
+                        user_ids: '',
+                        access_level: Gitlab::Access::GUEST
+
+          expect(response).to set_flash.to 'No users or groups specified.'
+          expect(response).to redirect_to(namespace_project_project_members_path(project.namespace, project))
+          expect(project.users).not_to include project_user
+        end
+      end
+    end
+  end
+
   describe 'DELETE destroy' do
     let(:member) { create(:project_member, :developer, project: project) }
 
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 47f89f744cb..ac862055ebc 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -265,10 +265,4 @@ describe Group, models: true do
 
     members
   end
-
-  describe '#web_url' do
-    it 'returns the canonical URL' do
-      expect(group.web_url).to include("groups/#{group.name}")
-    end
-  end
 end
diff --git a/spec/support/select2_helper.rb b/spec/support/select2_helper.rb
index 35cc51725c6..d30cc8ff9f2 100644
--- a/spec/support/select2_helper.rb
+++ b/spec/support/select2_helper.rb
@@ -17,9 +17,9 @@ module Select2Helper
     selector = options.fetch(:from)
 
     if options[:multiple]
-      execute_script("$('#{selector}').select2('val', ['#{value}'], true);")
+      execute_script("$('#{selector}').select2('val', ['#{value}']).trigger('change');")
     else
-      execute_script("$('#{selector}').select2('val', '#{value}', true);")
+      execute_script("$('#{selector}').select2('val', '#{value}').trigger('change');")
     end
   end
 end