Apply changes recomended by merge request coach

8 files changed, 29 insertions, 26 deletions

diff --git a/app/models/group.rb b/app/models/group.rb
index 9520db1bc0a..3f80c1373f1 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -416,6 +416,10 @@ class Group < Namespace
     super || ::Gitlab::CurrentSettings.default_project_creation
   end
 
+  def subgroup_creation_level
+    super || ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS
+  end
+
   private
 
   def update_two_factor_requirement
diff --git a/app/views/groups/_group_admin_settings.html.haml b/app/views/groups/_group_admin_settings.html.haml
index b8f632d11d3..733cb36cc3d 100644
--- a/app/views/groups/_group_admin_settings.html.haml
+++ b/app/views/groups/_group_admin_settings.html.haml
@@ -17,6 +17,12 @@
     = f.select :project_creation_level, options_for_select(::Gitlab::Access.project_creation_options, @group.project_creation_level), {}, class: 'form-control'
 
 .form-group.row
+  .col-sm-2.col-form-label
+    = f.label s_('SubgroupCreationlevel|Allowed to create subgroups')
+  .col-sm-10
+    = f.select :subgroup_creation_level, options_for_select(::Gitlab::Access.subgroup_creation_options, @group.subgroup_creation_level), {}, class: 'form-control'
+
+.form-group.row
   .col-sm-2.col-form-label.pt-0
     = f.label :require_two_factor_authentication, 'Two-factor authentication'
   .col-sm-10
diff --git a/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb b/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb
index 85ac89af46e..867ec3b7c91 100644
--- a/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb
+++ b/db/migrate/20190626175626_add_group_creation_level_to_namespaces.rb
@@ -7,18 +7,13 @@ class AddGroupCreationLevelToNamespaces < ActiveRecord::Migration[5.1]
   disable_ddl_transaction!
 
   def up
-    unless column_exists?(:namespaces, :subgroup_creation_level)
-      add_column_with_default(:namespaces,
-                              :subgroup_creation_level,
-                              :integer,
-                              default: 0)
-      change_column_default(:namespaces, :subgroup_creation_level, 1)
-    end
+    add_column(:namespaces, :subgroup_creation_level, :integer)
+    change_column_default(:namespaces,
+                          :subgroup_creation_level,
+                          ::Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS)
   end
 
   def down
-    if column_exists?(:namespaces, :subgroup_creation_level)
-      remove_column(:namespaces, :subgroup_creation_level)
-    end
+    remove_column(:namespaces, :subgroup_creation_level)
   end
 end
diff --git a/db/schema.rb b/db/schema.rb
index 0f5770e4eb1..79cd1a3a797 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -2119,7 +2119,7 @@ ActiveRecord::Schema.define(version: 2019_07_15_114644) do
     t.string "ldap_sync_status", default: "ready", null: false
     t.boolean "membership_lock", default: false
     t.integer "last_ci_minutes_usage_notification_level"
-    t.integer "subgroup_creation_level", default: 0, null: false
+    t.integer "subgroup_creation_level", default: 1
     t.index ["created_at"], name: "index_namespaces_on_created_at", using: :btree
     t.index ["custom_project_templates_group_id", "type"], name: "index_namespaces_on_custom_project_templates_group_id_and_type", where: "(custom_project_templates_group_id IS NOT NULL)", using: :btree
     t.index ["file_template_project_id"], name: "index_namespaces_on_file_template_project_id", using: :btree
diff --git a/spec/controllers/admin/groups_controller_spec.rb b/spec/controllers/admin/groups_controller_spec.rb
index 398f587bafe..1123563c1e3 100644
--- a/spec/controllers/admin/groups_controller_spec.rb
+++ b/spec/controllers/admin/groups_controller_spec.rb
@@ -70,13 +70,11 @@ describe Admin::GroupsController do
     end
 
     it 'updates the subgroup_creation_level successfully' do
-      OWNER = ::Gitlab::Access::OWNER_SUBGROUP_ACCESS
-
       expect do
         post :update,
              params: { id: group.to_param,
-                       group: { subgroup_creation_level: OWNER } }
-      end.to change { group.reload.subgroup_creation_level }.to(OWNER)
+                       group: { subgroup_creation_level: ::Gitlab::Access::OWNER_SUBGROUP_ACCESS } }
+      end.to change { group.reload.subgroup_creation_level }.to(::Gitlab::Access::OWNER_SUBGROUP_ACCESS)
     end
   end
 end
diff --git a/spec/features/admin/admin_groups_spec.rb b/spec/features/admin/admin_groups_spec.rb
index 735ca60f7da..35c384dd458 100644
--- a/spec/features/admin/admin_groups_spec.rb
+++ b/spec/features/admin/admin_groups_spec.rb
@@ -102,6 +102,15 @@ describe 'Admin Groups' do
       expect_selected_visibility(group.visibility_level)
     end
 
+    it 'shows the subgroup creation level dropdown populated with the group subgroup_creation_level value' do
+      group = create(:group, :private, :owner_subgroup_creation_only)
+
+      visit admin_group_edit_path(group)
+
+      expect(page).to have_select("group_subgroup_creation_level",
+                                  selected: ::Gitlab::Access.subgroup_creation_options.keys[group.subgroup_creation_level])
+    end
+
     it 'edit group path does not change group name', :js do
       group = create(:group, :private)
 
diff --git a/spec/features/groups/show_spec.rb b/spec/features/groups/show_spec.rb
index ef0e885ee5f..5096abadb79 100644
--- a/spec/features/groups/show_spec.rb
+++ b/spec/features/groups/show_spec.rb
@@ -71,7 +71,7 @@ describe 'Group show page' do
         sign_in(owner)
       end
 
-      context 'when subgroups are supported', :js, :nested_groups do
+      context 'when subgroups are supported', :nested_groups do
         before do
           allow(Group).to receive(:supports_nested_objects?) { true }
           visit path
@@ -101,7 +101,7 @@ describe 'Group show page' do
         sign_in(maintainer)
       end
 
-      context 'when subgroups are supported', :js, :nested_groups do
+      context 'when subgroups are supported', :nested_groups do
         before do
           allow(Group).to receive(:supports_nested_objects?) { true }
         end
diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb
index b4e6ddddfac..c5ff6cdbacd 100644
--- a/spec/services/groups/create_service_spec.rb
+++ b/spec/services/groups/create_service_spec.rb
@@ -1,4 +1,3 @@
-# coding: utf-8
 # frozen_string_literal: true
 
 require 'spec_helper'
@@ -88,14 +87,6 @@ describe Groups::CreateService, '#execute' do
 
         it { is_expected.to be_persisted }
       end
-
-      context 'as maintainer' do
-        before do
-          group.add_maintainer(user)
-        end
-
-        it { is_expected.to be_persisted }
-      end
     end
   end