Enhance subscriptions group controller tests

- enhance tests
- resolve linting issues

.rubocop_todo/gettext/static_identifier.yml

@@ -13,7 +13,6 @@ Gettext/StaticIdentifier:
     - 'app/services/users/banned_user_base_service.rb'
     - 'app/services/work_items/widgets/hierarchy_service/base_service.rb'
     - 'ee/app/controllers/admin/licenses_controller.rb'
-    - 'ee/app/controllers/subscriptions/groups_controller.rb'
     - 'ee/app/mailers/ee/emails/admin_notification.rb'
     - 'ee/app/mailers/emails/namespace_storage_usage_mailer.rb'
     - 'ee/app/models/ee/member.rb'

.rubocop_todo/layout/line_length.yml

@@ -1350,7 +1350,6 @@ Layout/LineLength:
     - 'ee/spec/controllers/projects/subscriptions_controller_spec.rb'
     - 'ee/spec/controllers/projects/vulnerability_feedback_controller_spec.rb'
     - 'ee/spec/controllers/projects_controller_spec.rb'
-    - 'ee/spec/controllers/subscriptions/groups_controller_spec.rb'
     - 'ee/spec/controllers/subscriptions_controller_spec.rb'
     - 'ee/spec/elastic/migrate/migration_shared_examples.rb'
     - 'ee/spec/factories/ci/builds.rb'

.rubocop_todo/rspec/before_all_role_assignment.yml

@@ -61,7 +61,6 @@ RSpec/BeforeAllRoleAssignment:
     - 'ee/spec/controllers/projects_controller_spec.rb'
     - 'ee/spec/controllers/registrations/groups_controller_spec.rb'
     - 'ee/spec/controllers/security/projects_controller_spec.rb'
-    - 'ee/spec/controllers/subscriptions/groups_controller_spec.rb'
     - 'ee/spec/controllers/subscriptions_controller_spec.rb'
     - 'ee/spec/features/admin/admin_sends_notification_spec.rb'
     - 'ee/spec/features/analytics/code_analytics_spec.rb'

.rubocop_todo/style/empty_method.yml

@@ -91,7 +91,6 @@ Style/EmptyMethod:
     - 'ee/app/controllers/projects/security/dast_scanner_profiles_controller.rb'
     - 'ee/app/controllers/projects/security/dast_site_profiles_controller.rb'
     - 'ee/app/controllers/projects/security/sast_configuration_controller.rb'
-    - 'ee/app/controllers/subscriptions/groups_controller.rb'
     - 'ee/app/models/ee/epic.rb'
     - 'ee/app/services/feature_flag_issues/destroy_service.rb'
     - 'ee/db/geo/migrate/20170906174622_remove_duplicates_from_project_registry.rb'

.rubocop_todo/style/format_string.yml

@@ -170,7 +170,6 @@ Style/FormatString:
     - 'ee/app/controllers/groups/saml_group_links_controller.rb'
     - 'ee/app/controllers/groups/sso_controller.rb'
     - 'ee/app/controllers/projects/requirements_management/requirements_controller.rb'
-    - 'ee/app/controllers/subscriptions/groups_controller.rb'
     - 'ee/app/helpers/admin/emails_helper.rb'
     - 'ee/app/helpers/billing_plans_helper.rb'
     - 'ee/app/helpers/ee/application_helper.rb'

ee/app/controllers/subscriptions/groups_controller.rb

@@ -12,15 +12,14 @@ class GroupsController < ApplicationController
     feature_category :purchase
     urgency :low
 
-    def edit
-    end
+    def edit; end
 
     def update
       if Groups::UpdateService.new(@group, current_user, group_params).execute
-        notice = if params[:new_user] == 'true'
-                   _('Welcome to GitLab, %{first_name}!' % { first_name: current_user.first_name })
+        notice = if ::Gitlab::Utils.to_boolean(params[:new_user])
+                   format(_('Welcome to GitLab, %{first_name}!'), first_name: current_user.first_name)
                  else
-                   _('Subscription successfully applied to "%{group_name}"' % { group_name: @group.name })
+                   format(_('Subscription successfully applied to "%{group_name}"'), group_name: @group.name)
                  end
 
         redirect_to group_path(@group), notice: notice

ee/spec/controllers/subscriptions/groups_controller_spec.rb

@@ -4,7 +4,7 @@
 
 RSpec.describe Subscriptions::GroupsController, feature_category: :purchase do
   let_it_be(:user) { create(:user) }
-  let_it_be(:group) { create(:group) }
+  let_it_be(:group) { create(:group, :public) }
 
   describe 'GET #edit' do
     subject { get :edit, params: { id: group.to_param } }
@@ -23,9 +23,12 @@
     end
 
     context 'with an authenticated user' do
+      before_all do
+        group.add_owner(user)
+      end
+
       before do
         sign_in(user)
-        group.add_owner(user)
       end
 
       it { is_expected.to have_gitlab_http_status(:ok) }
@@ -33,13 +36,29 @@
   end
 
   describe 'PUT #update' do
-    subject { post :update, params: { id: group.to_param, group: params } }
+    subject(:put_update) { put :update, params: { id: group.to_param, group: params } }
 
     let(:params) { { name: 'New name', path: 'new-path' } }
 
     context 'with an unauthenticated user' do
       it { is_expected.to have_gitlab_http_status(:redirect) }
       it { is_expected.to redirect_to(new_user_session_path) }
+
+      it 'does not update the name' do
+        expect { put_update }.not_to change { group.reload.name }
+      end
+
+      it 'does not update the path' do
+        expect { put_update }.not_to change { group.reload.path }
+      end
+
+      context 'for visibility change' do
+        let(:params) { { visibility_level: Gitlab::VisibilityLevel::PRIVATE } }
+
+        it 'does not update visibility' do
+          expect { put_update }.not_to change { group.reload.visibility_level }
+        end
+      end
     end
 
     context 'with an authenticated user who is not a group owner' do
@@ -50,46 +69,62 @@
       it { is_expected.to have_gitlab_http_status(:not_found) }
 
       it 'does not update the name' do
-        expect { subject }.not_to change { group.reload.name }
+        expect { put_update }.not_to change { group.reload.name }
+      end
+
+      it 'does not update the path' do
+        expect { put_update }.not_to change { group.reload.path }
+      end
+
+      context 'for visibility change' do
+        let(:params) { { visibility_level: Gitlab::VisibilityLevel::PRIVATE } }
+
+        it 'does not update visibility' do
+          expect { put_update }.not_to change { group.reload.visibility_level }
+        end
       end
     end
 
     context 'with an authenticated user' do
       let(:params) { { name: 'New name', path: 'new-path', visibility_level: Gitlab::VisibilityLevel::PRIVATE } }
 
+      before_all do
+        group.add_owner(user)
+      end
+
       before do
         sign_in(user)
-        group.add_owner(user)
       end
 
+      it { is_expected.to have_gitlab_http_status(:redirect) }
+      it { is_expected.to redirect_to('/new-path') }
+
       it 'updates the name' do
-        expect { subject }.to change { group.reload.name }.to('New name')
+        expect { put_update }.to change { group.reload.name }.to('New name')
       end
 
       it 'updates the path' do
-        expect { subject }.to change { group.reload.path }.to('new-path')
+        expect { put_update }.to change { group.reload.path }.to('new-path')
       end
 
       it 'updates the visibility_level' do
-        expect { subject }.to change { group.reload.visibility_level }.from(Gitlab::VisibilityLevel::PUBLIC).to(Gitlab::VisibilityLevel::PRIVATE)
-      end
-
-      it 'redirects to the group path' do
-        expect(subject).to have_gitlab_http_status(:redirect)
-        expect(subject).to redirect_to('/new-path')
+        expect do
+          put_update
+        end.to change { group.reload.visibility_level }.from(Gitlab::VisibilityLevel::PUBLIC)
+                                                       .to(Gitlab::VisibilityLevel::PRIVATE)
       end
 
       it 'sets flash notice' do
-        subject
+        put_update
 
         expect(controller).to set_flash[:notice].to('Subscription successfully applied to "New name"')
       end
 
       context 'with new_user param' do
-        subject { post :update, params: { id: group.to_param, group: params, new_user: 'true' } }
+        subject(:put_update) { put :update, params: { id: group.to_param, group: params, new_user: 'true' } }
 
         it 'sets flash notice' do
-          subject
+          put_update
 
           expect(controller).to set_flash[:notice].to("Welcome to GitLab, #{user.first_name}!")
         end
@@ -97,25 +132,26 @@
     end
 
     context 'when the group cannot be saved' do
+      before_all do
+        group.add_owner(user)
+      end
+
       before do
         sign_in(user)
-        group.add_owner(user)
       end
 
       let(:params) { { name: '', path: '' } }
 
       it 'does not update the name' do
-        expect { subject }.not_to change { group.reload.name }
+        expect { put_update }.not_to change { group.reload.name }
       end
 
       it 'does not update the path' do
-        expect { subject }.not_to change { group.reload.path }
+        expect { put_update }.not_to change { group.reload.path }
       end
 
-      it 're-renders the edit template' do
-        expect(subject).to have_gitlab_http_status(:ok)
-        expect(subject).to render_template(:edit)
-      end
+      it { is_expected.to have_gitlab_http_status(:ok) }
+      it { is_expected.to render_template(:edit) }
     end
   end
 end

spec/support/rspec_order_todo.yml

@@ -166,7 +166,6 @@
 - './ee/spec/controllers/security/vulnerabilities_controller_spec.rb'
 - './ee/spec/controllers/sitemap_controller_spec.rb'
 - './ee/spec/controllers/subscriptions_controller_spec.rb'
-- './ee/spec/controllers/subscriptions/groups_controller_spec.rb'
 - './ee/spec/controllers/trial_registrations_controller_spec.rb'
 - './ee/spec/controllers/users_controller_spec.rb'
 - './ee/spec/db/production/license_spec.rb'