From 9410f215eab0ba49051d2a00f0b4174f5dc13a6f Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 26 Dec 2016 11:56:19 +0200 Subject: [PATCH 1/3] Add nested groups support to the Groups::CreateService Signed-off-by: Dmitriy Zaporozhets --- app/services/groups/create_service.rb | 13 +++++++++ spec/services/groups/create_service_spec.rb | 31 ++++++++++++++++++--- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/app/services/groups/create_service.rb b/app/services/groups/create_service.rb index 2bccd584dde..9a630aee626 100644 --- a/app/services/groups/create_service.rb +++ b/app/services/groups/create_service.rb @@ -12,6 +12,19 @@ module Groups return @group end + parent_id = params[:parent_id] + + if parent_id + parent = Group.find(parent_id) + + unless can?(current_user, :admin_group, parent) + @group.parent_id = nil + @group.errors.add(:parent_id, 'manage access required to create subgroup') + + return @group + end + end + @group.name ||= @group.path.dup @group.save @group.add_owner(current_user) diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb index 71a0b8e2a12..14717a7455d 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/spec/services/groups/create_service_spec.rb @@ -1,11 +1,12 @@ require 'spec_helper' -describe Groups::CreateService, services: true do - let!(:user) { create(:user) } +describe Groups::CreateService, '#execute', services: true do + let!(:user) { create(:user) } let!(:group_params) { { path: "group_path", visibility_level: Gitlab::VisibilityLevel::PUBLIC } } - describe "execute" do - let!(:service) { described_class.new(user, group_params ) } + describe 'visibility level restrictions' do + let!(:service) { described_class.new(user, group_params) } + subject { service.execute } context "create groups without restricted visibility level" do @@ -14,7 +15,29 @@ describe Groups::CreateService, services: true do context "cannot create group with restricted visibility level" do before { allow_any_instance_of(ApplicationSetting).to receive(:restricted_visibility_levels).and_return([Gitlab::VisibilityLevel::PUBLIC]) } + it { is_expected.not_to be_persisted } end end + + describe 'creating subgroup' do + let!(:group) { create(:group) } + let!(:service) { described_class.new(user, group_params.merge(parent_id: group.id)) } + + subject { service.execute } + + context 'as group owner' do + before { group.add_owner(user) } + + it { is_expected.to be_persisted } + end + + context 'as guest' do + it 'does not save group and returns an error' do + is_expected.not_to be_persisted + expect(subject.errors[:parent_id].first).to eq('manage access required to create subgroup') + expect(subject.parent_id).to be_nil + end + end + end end From 10de4e3bb612a529480c93da53849d95e98a8633 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 26 Dec 2016 12:51:48 +0200 Subject: [PATCH 2/3] Show nested groups tab on group page Signed-off-by: Dmitriy Zaporozhets --- app/controllers/groups_controller.rb | 2 ++ app/views/groups/show.html.haml | 10 ++++++++++ changelogs/unreleased/dz-nested-group-misc.yml | 4 ++++ spec/features/groups_spec.rb | 13 +++++++++++++ 4 files changed, 29 insertions(+) create mode 100644 changelogs/unreleased/dz-nested-group-misc.yml diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index efe9c001bcf..e061df2f819 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -42,6 +42,8 @@ class GroupsController < Groups::ApplicationController @notification_setting = current_user.notification_settings_for(group) end + @nested_groups = group.children + setup_projects respond_to do |format| diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml index 52ce26a20b1..a3cd333373e 100644 --- a/app/views/groups/show.html.haml +++ b/app/views/groups/show.html.haml @@ -32,6 +32,10 @@ %li = link_to "#shared", 'data-toggle' => 'tab' do Shared Projects + - if @nested_groups.present? + %li + = link_to "#groups", 'data-toggle' => 'tab' do + Nested Groups .nav-controls = form_tag request.path, method: :get, class: 'project-filter-form', id: 'project-filter-form' do |f| = search_field_tag :filter_projects, nil, placeholder: 'Filter by name', class: 'projects-list-filter form-control', spellcheck: false @@ -47,3 +51,9 @@ - if @shared_projects.present? .tab-pane#shared = render "shared_projects", projects: @shared_projects + + - if @nested_groups.present? + .tab-pane#groups + %ul.content-list + - @nested_groups.each do |group| + = render 'shared/groups/group', group: group diff --git a/changelogs/unreleased/dz-nested-group-misc.yml b/changelogs/unreleased/dz-nested-group-misc.yml new file mode 100644 index 00000000000..9c9d0b1c644 --- /dev/null +++ b/changelogs/unreleased/dz-nested-group-misc.yml @@ -0,0 +1,4 @@ +--- +title: Show nested groups tab on group page +merge_request: 8308 +author: diff --git a/spec/features/groups_spec.rb b/spec/features/groups_spec.rb index 4b19886274e..d865a71f04b 100644 --- a/spec/features/groups_spec.rb +++ b/spec/features/groups_spec.rb @@ -107,4 +107,17 @@ feature 'Group', feature: true do expect(page).to have_css('.group-home-desc a[rel]') end end + + describe 'group page with nested groups', js: true do + let!(:group) { create(:group) } + let!(:nested_group) { create(:group, parent: group) } + let!(:path) { group_path(group) } + + it 'has nested groups tab with nested groups inside' do + visit path + click_link 'Nested Groups' + + expect(page).to have_content(nested_group.full_name) + end + end end From 283e868ef523185b0ee314b9e2164599780d888b Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 29 Dec 2016 19:18:05 +0200 Subject: [PATCH 3/3] Refactor nested group related code * Simplify code around group parent access check * Rename 'Nested groups' to 'Subgroups' tab at group#show page Signed-off-by: Dmitriy Zaporozhets --- app/services/groups/create_service.rb | 14 ++++---------- app/views/groups/show.html.haml | 5 ++--- spec/features/groups_spec.rb | 2 +- 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/app/services/groups/create_service.rb b/app/services/groups/create_service.rb index 9a630aee626..febeb661fb5 100644 --- a/app/services/groups/create_service.rb +++ b/app/services/groups/create_service.rb @@ -12,17 +12,11 @@ module Groups return @group end - parent_id = params[:parent_id] + if @group.parent && !can?(current_user, :admin_group, @group.parent) + @group.parent = nil + @group.errors.add(:parent_id, 'manage access required to create subgroup') - if parent_id - parent = Group.find(parent_id) - - unless can?(current_user, :admin_group, parent) - @group.parent_id = nil - @group.errors.add(:parent_id, 'manage access required to create subgroup') - - return @group - end + return @group end @group.name ||= @group.path.dup diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml index a3cd333373e..9ef88f233c5 100644 --- a/app/views/groups/show.html.haml +++ b/app/views/groups/show.html.haml @@ -35,7 +35,7 @@ - if @nested_groups.present? %li = link_to "#groups", 'data-toggle' => 'tab' do - Nested Groups + Subgroups .nav-controls = form_tag request.path, method: :get, class: 'project-filter-form', id: 'project-filter-form' do |f| = search_field_tag :filter_projects, nil, placeholder: 'Filter by name', class: 'projects-list-filter form-control', spellcheck: false @@ -55,5 +55,4 @@ - if @nested_groups.present? .tab-pane#groups %ul.content-list - - @nested_groups.each do |group| - = render 'shared/groups/group', group: group + = render partial: 'shared/groups/group', collection: @nested_groups diff --git a/spec/features/groups_spec.rb b/spec/features/groups_spec.rb index d865a71f04b..a515c92db37 100644 --- a/spec/features/groups_spec.rb +++ b/spec/features/groups_spec.rb @@ -115,7 +115,7 @@ feature 'Group', feature: true do it 'has nested groups tab with nested groups inside' do visit path - click_link 'Nested Groups' + click_link 'Subgroups' expect(page).to have_content(nested_group.full_name) end