From 35ced4dae480d61ddc4d73eb4695626ecc419e9c Mon Sep 17 00:00:00 2001 From: barthc Date: Thu, 1 Sep 2016 18:23:39 +0100 Subject: [PATCH] fix group links 404 --- app/assets/javascripts/api.js | 3 +- app/assets/javascripts/groups_select.js | 5 ++- app/assets/javascripts/project_select.js | 4 +- app/assets/javascripts/search.js | 2 +- .../projects/group_links_controller.rb | 22 +++++++---- app/helpers/selects_helper.rb | 6 +-- .../projects/group_links/index.html.haml | 4 +- lib/api/groups.rb | 3 ++ .../projects/group_links_controller_spec.rb | 37 ++++++++++++++++++- spec/requests/api/groups_spec.rb | 10 +++++ 10 files changed, 76 insertions(+), 20 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) + group = Group.find(params[:link_group_id]) if params[:link_group_id].present? - project.project_group_links.create( - group: group, - group_access: params[:link_group_access], - expires_at: params[:expires_at] - ) + 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