fix group links 404

10 files changed, 77 insertions, 21 deletions

diff --git a/app/assets/javascripts/api.js b/app/assets/javascripts/api.js
index 1cd2302111e..4f04392513f 100644
--- a/app/assets/javascripts/api.js
+++ b/app/assets/javascripts/api.js
@@ -23,12 +23,13 @@
     },
     // Return groups list. Filtered by query
     // Only active groups retrieved
-    groups: function(query, skip_ldap, callback) {
+    groups: function(query, skip_ldap, skip_groups, callback) {
       var url = Api.buildUrl(Api.groupsPath);
       return $.ajax({
         url: url,
         data: {
           search: query,
+          skip_groups: skip_groups,
           per_page: 20
         },
         dataType: "json"
diff --git a/app/assets/javascripts/groups_select.js b/app/assets/javascripts/groups_select.js
index 7c2eebcdd44..5f06186504b 100644
--- a/app/assets/javascripts/groups_select.js
+++ b/app/assets/javascripts/groups_select.js
@@ -5,14 +5,15 @@
     function GroupsSelect() {
       $('.ajax-groups-select').each((function(_this) {
         return function(i, select) {
-          var skip_ldap;
+          var skip_ldap, skip_groups;
           skip_ldap = $(select).hasClass('skip_ldap');
+          skip_groups = $(select).data('skip-groups') || [];
           return $(select).select2({
             placeholder: "Search for a group",
             multiple: $(select).hasClass('multiselect'),
             minimumInputLength: 0,
             query: function(query) {
-              return Api.groups(query.term, skip_ldap, function(groups) {
+              return Api.groups(query.term, skip_ldap, skip_groups, function(groups) {
                 var data;
                 data = {
                   results: groups
diff --git a/app/assets/javascripts/project_select.js b/app/assets/javascripts/project_select.js
index 20b147500cf..4239ed2f889 100644
--- a/app/assets/javascripts/project_select.js
+++ b/app/assets/javascripts/project_select.js
@@ -23,7 +23,7 @@
                   data = groups.concat(projects);
                   return finalCallback(data);
                 };
-                return Api.groups(term, false, groupsCallback);
+                return Api.groups(term, false, false, groupsCallback);
               };
             } else {
               projectsCallback = finalCallback;
@@ -72,7 +72,7 @@
                     data = groups.concat(projects);
                     return finalCallback(data);
                   };
-                  return Api.groups(query.term, false, groupsCallback);
+                  return Api.groups(query.term, false, false, groupsCallback);
                 };
               } else {
                 projectsCallback = finalCallback;
diff --git a/app/assets/javascripts/search.js b/app/assets/javascripts/search.js
index d34346f862b..8074a94f33e 100644
--- a/app/assets/javascripts/search.js
+++ b/app/assets/javascripts/search.js
@@ -10,7 +10,7 @@
         filterable: true,
         fieldName: 'group_id',
         data: function(term, callback) {
-          return Api.groups(term, null, function(data) {
+          return Api.groups(term, false, false, function(data) {
             data.unshift({
               name: 'Any'
             });
diff --git a/app/controllers/projects/group_links_controller.rb b/app/controllers/projects/group_links_controller.rb
index d0c4550733c..7a7475a7345 100644
--- a/app/controllers/projects/group_links_controller.rb
+++ b/app/controllers/projects/group_links_controller.rb
@@ -4,17 +4,25 @@ class Projects::GroupLinksController < Projects::ApplicationController
 
   def index
     @group_links = project.project_group_links.all
+
+    @skip_groups = @group_links.pluck(:group_id)
+    @skip_groups << project.group.try(:id)
   end
 
   def create
-    group = Group.find(params[:link_group_id])
-    return render_404 unless can?(current_user, :read_group, group)
-
-    project.project_group_links.create(
-      group: group,
-      group_access: params[:link_group_access],
-      expires_at: params[:expires_at]
-    )
+    group = Group.find(params[:link_group_id]) if params[:link_group_id].present?
+
+    if group
+      return render_404 unless can?(current_user, :read_group, group)
+
+      project.project_group_links.create(
+        group: group,
+        group_access: params[:link_group_access],
+        expires_at: params[:expires_at]
+      )
+    else
+      flash[:alert] = 'Please select a group.'
+    end
 
     redirect_to namespace_project_group_links_path(project.namespace, project)
   end
diff --git a/app/helpers/selects_helper.rb b/app/helpers/selects_helper.rb
index 5f27e33c6ad..8706876ae4a 100644
--- a/app/helpers/selects_helper.rb
+++ b/app/helpers/selects_helper.rb
@@ -49,12 +49,10 @@ module SelectsHelper
   end
 
   def select2_tag(id, opts = {})
-    css_class = ''
-    css_class << 'multiselect ' if opts[:multiple]
-    css_class << (opts[:class] || '')
+    opts[:class] << ' multiselect' if opts[:multiple]
     value = opts[:selected] || ''
 
-    hidden_field_tag(id, value, class: css_class)
+    hidden_field_tag(id, value, opts)
   end
 
   private
diff --git a/app/views/projects/group_links/index.html.haml b/app/views/projects/group_links/index.html.haml
index ca700cb3a3b..4c5dd9b88bf 100644
--- a/app/views/projects/group_links/index.html.haml
+++ b/app/views/projects/group_links/index.html.haml
@@ -8,10 +8,10 @@
   .col-lg-9
     %h5.prepend-top-0
       Set a group to share
-    = form_tag namespace_project_group_links_path(@project.namespace, @project), method: :post do
+    = form_tag namespace_project_group_links_path(@project.namespace, @project), class: 'js-requires-input', method: :post do
       .form-group
         = label_tag :link_group_id, "Group", class: "label-light"
-        = groups_select_tag(:link_group_id, skip_group: @project.group.try(:path))
+        = groups_select_tag(:link_group_id, data: { skip_groups: @skip_groups }, required: true)
       .form-group
         = label_tag :link_group_access, "Max access level", class: "label-light"
         .select-wrapper
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index 953fa474e88..bfb89475025 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -6,6 +6,8 @@ module API
     resource :groups do
       # Get a groups list
       #
+      # Parameters:
+      #   skip_groups (optional) - Array of group ids to exclude from list
       # Example Request:
       #  GET /groups
       get do
@@ -16,6 +18,7 @@ module API
                   end
 
         @groups = @groups.search(params[:search]) if params[:search].present?
+        @groups = @groups.where.not(id: params[:skip_groups]) if params[:skip_groups].present?
         @groups = paginate @groups
         present @groups, with: Entities::Group
       end
diff --git a/spec/controllers/projects/group_links_controller_spec.rb b/spec/controllers/projects/group_links_controller_spec.rb
index fbe8758dda7..b9d9117c928 100644
--- a/spec/controllers/projects/group_links_controller_spec.rb
+++ b/spec/controllers/projects/group_links_controller_spec.rb
@@ -1,8 +1,9 @@
 require 'spec_helper'
 
 describe Projects::GroupLinksController do
-  let(:project) { create(:project, :private) }
   let(:group) { create(:group, :private) }
+  let(:group2) { create(:group, :private) }
+  let(:project) { create(:project, :private, group: group2) }
   let(:user) { create(:user) }
 
   before do
@@ -46,5 +47,39 @@ describe Projects::GroupLinksController do
         expect(group.shared_projects).not_to include project
       end
     end
+
+    context 'when project group id equal link group id' do
+      before do
+        post(:create, namespace_id: project.namespace.to_param,
+                      project_id: project.to_param,
+                      link_group_id: group2.id,
+                      link_group_access: ProjectGroupLink.default_access)
+      end
+
+      it 'does not share project with selected group' do
+        expect(group2.shared_projects).not_to include project
+      end
+
+      it 'redirects to project group links page' do
+        expect(response).to redirect_to(
+          namespace_project_group_links_path(project.namespace, project)
+        )
+      end
+    end
+
+    context 'when link group id is not present' do
+      before do
+        post(:create, namespace_id: project.namespace.to_param,
+                      project_id: project.to_param,
+                      link_group_access: ProjectGroupLink.default_access)
+      end
+
+      it 'redirects to project group links page' do
+        expect(response).to redirect_to(
+          namespace_project_group_links_path(project.namespace, project)
+        )
+        expect(flash[:alert]).to eq('Please select a group.')
+      end
+    end
   end
 end
diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb
index 1f68ef1af8f..3ba257256a0 100644
--- a/spec/requests/api/groups_spec.rb
+++ b/spec/requests/api/groups_spec.rb
@@ -45,6 +45,16 @@ describe API::API, api: true  do
         expect(json_response.length).to eq(2)
       end
     end
+
+    context "when using skip_groups in request" do
+      it "returns all groups excluding skipped groups" do
+        get api("/groups", admin), skip_groups: [group2.id]
+
+        expect(response).to have_http_status(200)
+        expect(json_response).to be_an Array
+        expect(json_response.length).to eq(1)
+      end
+    end
   end
 
   describe "GET /groups/:id" do