Skip to content

G
gitlab-ce
Commit d55822ac authored by Stan Hu's avatar Stan Hu
Browse files
Options

Merge branch '328806-improve-clarity-in-the-users-buildservice-3' into 'master' 

Optimize Users::BuildService spec [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!61734
parents 20ec7721 9af4215e
Hide whitespace changes
Inline Side-by-side
Showing with 134 additions and 116 deletions
ee/spec/services/ee/users/build_service_spec.rb
View file @ d55822ac
......@@ -34,11 +34,13 @@ RSpec.describe Users::BuildService do
end
context 'with an admin user' do
let!(:admin_user) { create(:admin) }
let_it_be(:admin_user) { create(:admin) }
let(:service) { described_class.new(admin_user, ActionController::Parameters.new(params).permit!) }
context 'allowed params' do
let(:provider) { create(:saml_provider) }
let_it_be(:provider) { create(:saml_provider) }
let(:identity_params) { { extern_uid: 'uid', provider: 'group_saml', saml_provider_id: provider.id } }
before do
......
spec/services/users/build_service_spec.rb
View file @ d55822ac
......@@ -6,105 +6,76 @@ RSpec.describe Users::BuildService do
using RSpec::Parameterized::TableSyntax
describe '#execute' do
let(:params) { build_stubbed(:user).slice(:first_name, :last_name, :username, :email, :password) }
context 'with an admin user' do
let(:params) { build_stubbed(:user).slice(:name, :username, :email, :password) }
let_it_be(:current_user) { nil }
let(:admin_user) { create(:admin) }
let(:service) { described_class.new(admin_user, ActionController::Parameters.new(params).permit!) }
let(:params) { build_stubbed(:user).slice(:first_name, :last_name, :username, :email, :password) }
let(:service) { described_class.new(current_user, params) }
it 'returns a valid user' do
expect(service.execute).to be_valid
end
shared_examples_for 'common build items' do
it { is_expected.to be_valid }
it 'sets the created_by_id' do
expect(service.execute.created_by_id).to eq(admin_user.id)
expect(user.created_by_id).to eq(current_user&.id)
end
context 'calls the UpdateCanonicalEmailService' do
specify do
expect(Users::UpdateCanonicalEmailService).to receive(:new).and_call_original
it 'calls UpdateCanonicalEmailService' do
expect(Users::UpdateCanonicalEmailService).to receive(:new).and_call_original
service.execute
end
user
end
context 'allowed params' do
let(:params) do
{
access_level: 1,
admin: 1,
avatar: anything,
bio: 1,
can_create_group: 1,
color_scheme_id: 1,
email: 1,
external: 1,
force_random_password: 1,
hide_no_password: 1,
hide_no_ssh_key: 1,
linkedin: 1,
name: 1,
password: 1,
password_automatically_set: 1,
password_expires_at: 1,
projects_limit: 1,
remember_me: 1,
skip_confirmation: 1,
skype: 1,
theme_id: 1,
twitter: 1,
username: 1,
website_url: 1,
private_profile: 1,
organization: 1,
location: 1,
public_email: 1
}
end
context 'when user_type is provided' do
context 'when project_bot' do
before do
params.merge!({ user_type: :project_bot })
end
it 'sets all allowed attributes' do
admin_user # call first so the admin gets created before setting `expect`
it { expect(user.project_bot?).to be true }
end
expect(User).to receive(:new).with(hash_including(params)).and_call_original
context 'when not a project_bot' do
before do
params.merge!({ user_type: :alert_bot })
end
service.execute
it { expect(user).to be_human }
end
end
end
shared_examples_for 'current user not admin' do
context 'with "user_default_external" application setting' do
where(:user_default_external, :external, :email, :user_default_internal_regex, :result) do
true | nil | 'fl@example.com' | nil | true
true | true | 'fl@example.com' | nil | true
true | false | 'fl@example.com' | nil | false
true | false | 'fl@example.com' | nil | true # admin difference
true | nil | 'fl@example.com' | '' | true
true | true | 'fl@example.com' | '' | true
true | false | 'fl@example.com' | '' | false
true | false | 'fl@example.com' | '' | true # admin difference
true | nil | 'fl@example.com' | '^(?:(?!\.ext@).)*$\r?' | false
true | true | 'fl@example.com' | '^(?:(?!\.ext@).)*$\r?' | true
true | true | 'fl@example.com' | '^(?:(?!\.ext@).)*$\r?' | false # admin difference
true | false | 'fl@example.com' | '^(?:(?!\.ext@).)*$\r?' | false
true | nil | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | true
true | true | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | true
true | false | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | false
true | false | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | true # admin difference
false | nil | 'fl@example.com' | nil | false
false | true | 'fl@example.com' | nil | true
false | true | 'fl@example.com' | nil | false # admin difference
false | false | 'fl@example.com' | nil | false
false | nil | 'fl@example.com' | '' | false
false | true | 'fl@example.com' | '' | true
false | true | 'fl@example.com' | '' | false # admin difference
false | false | 'fl@example.com' | '' | false
false | nil | 'fl@example.com' | '^(?:(?!\.ext@).)*$\r?' | false
false | true | 'fl@example.com' | '^(?:(?!\.ext@).)*$\r?' | true
false | true | 'fl@example.com' | '^(?:(?!\.ext@).)*$\r?' | false # admin difference
false | false | 'fl@example.com' | '^(?:(?!\.ext@).)*$\r?' | false
false | nil | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | false
false | true | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | true
false | true | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | false # admin difference
false | false | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | false
end
......@@ -116,40 +87,11 @@ RSpec.describe Users::BuildService do
params.merge!({ external: external, email: email }.compact)
end
subject(:user) { service.execute }
it 'correctly sets user.external' do
it 'sets the value of Gitlab::CurrentSettings.user_default_external' do
expect(user.external).to eq(result)
end
end
end
end
context 'with non admin user' do
let(:user) { create(:user) }
let(:service) { described_class.new(user, params) }
it 'raises AccessDeniedError exception' do
expect { service.execute }.to raise_error Gitlab::Access::AccessDeniedError
end
context 'when authorization is skipped' do
subject(:built_user) { service.execute(skip_authorization: true) }
it { is_expected.to be_valid }
it 'sets the created_by_id' do
expect(built_user.created_by_id).to eq(user.id)
end
end
end
context 'with nil user' do
let(:service) { described_class.new(nil, params) }
it 'returns a valid user' do
expect(service.execute).to be_valid
end
context 'when "send_user_confirmation_email" application setting is true' do
before do
......@@ -157,7 +99,7 @@ RSpec.describe Users::BuildService do
end
it 'does not confirm the user' do
expect(service.execute).not_to be_confirmed
expect(user).not_to be_confirmed
end
end
......@@ -167,27 +109,103 @@ RSpec.describe Users::BuildService do
end
it 'confirms the user' do
expect(service.execute).to be_confirmed
expect(user).to be_confirmed
end
end
context 'when user_type is provided' do
subject(:user) { service.execute }
context 'with allowed params' do
let(:params) do
{
email: 1,
name: 1,
password: 1,
password_automatically_set: 1,
username: 1,
user_type: 'project_bot'
}
end
context 'when project_bot' do
before do
params.merge!({ user_type: :project_bot })
end
it 'sets all allowed attributes' do
expect(User).to receive(:new).with(hash_including(params)).and_call_original
it { expect(user.project_bot?).to be true }
user
end
end
end
context 'when not a project_bot' do
before do
params.merge!({ user_type: :alert_bot })
end
context 'with nil current_user' do
subject(:user) { service.execute }
it { expect(user).to be_human }
it_behaves_like 'common build items'
it_behaves_like 'current user not admin'
end
context 'with non admin current_user' do
let_it_be(:current_user) { create(:user) }
let(:service) { described_class.new(current_user, params) }
subject(:user) { service.execute(skip_authorization: true) }
it 'raises AccessDeniedError exception when authorization is not skipped' do
expect { service.execute }.to raise_error Gitlab::Access::AccessDeniedError
end
it_behaves_like 'common build items'
it_behaves_like 'current user not admin'
end
context 'with an admin current_user' do
let_it_be(:current_user) { create(:admin) }
let(:params) { build_stubbed(:user).slice(:name, :username, :email, :password) }
let(:service) { described_class.new(current_user, ActionController::Parameters.new(params).permit!) }
subject(:user) { service.execute }
it_behaves_like 'common build items'
context 'with allowed params' do
let(:params) do
{
access_level: 1,
admin: 1,
avatar: anything,
bio: 1,
can_create_group: 1,
color_scheme_id: 1,
email: 1,
external: 1,
force_random_password: 1,
hide_no_password: 1,
hide_no_ssh_key: 1,
linkedin: 1,
name: 1,
password: 1,
password_automatically_set: 1,
password_expires_at: 1,
projects_limit: 1,
remember_me: 1,
skip_confirmation: 1,
skype: 1,
theme_id: 1,
twitter: 1,
username: 1,
website_url: 1,
private_profile: 1,
organization: 1,
location: 1,
public_email: 1,
user_type: 'project_bot',
note: 1,
view_diffs_file_by_file: 1
}
end
it 'sets all allowed attributes' do
expect(User).to receive(:new).with(hash_including(params)).and_call_original
service.execute
end
end
......@@ -195,34 +213,34 @@ RSpec.describe Users::BuildService do
where(:user_default_external, :external, :email, :user_default_internal_regex, :result) do
true | nil | 'fl@example.com' | nil | true
true | true | 'fl@example.com' | nil | true
true | false | 'fl@example.com' | nil | true
true | false | 'fl@example.com' | nil | false # admin difference
true | nil | 'fl@example.com' | '' | true
true | true | 'fl@example.com' | '' | true
true | false | 'fl@example.com' | '' | true
true | false | 'fl@example.com' | '' | false # admin difference
true | nil | 'fl@example.com' | '^(?:(?!\.ext@).)*$\r?' | false
true | true | 'fl@example.com' | '^(?:(?!\.ext@).)*$\r?' | false
true | true | 'fl@example.com' | '^(?:(?!\.ext@).)*$\r?' | true # admin difference
true | false | 'fl@example.com' | '^(?:(?!\.ext@).)*$\r?' | false
true | nil | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | true
true | true | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | true
true | false | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | true
true | false | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | false # admin difference
false | nil | 'fl@example.com' | nil | false
false | true | 'fl@example.com' | nil | false
false | true | 'fl@example.com' | nil | true # admin difference
false | false | 'fl@example.com' | nil | false
false | nil | 'fl@example.com' | '' | false
false | true | 'fl@example.com' | '' | false
false | true | 'fl@example.com' | '' | true # admin difference
false | false | 'fl@example.com' | '' | false
false | nil | 'fl@example.com' | '^(?:(?!\.ext@).)*$\r?' | false
false | true | 'fl@example.com' | '^(?:(?!\.ext@).)*$\r?' | false
false | true | 'fl@example.com' | '^(?:(?!\.ext@).)*$\r?' | true # admin difference
false | false | 'fl@example.com' | '^(?:(?!\.ext@).)*$\r?' | false
false | nil | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | false
false | true | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | false
false | true | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | true # admin difference
false | false | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | false
end
......@@ -234,8 +252,6 @@ RSpec.describe Users::BuildService do
params.merge!({ external: external, email: email }.compact)
end
subject(:user) { service.execute }
it 'sets the value of Gitlab::CurrentSettings.user_default_external' do
expect(user.external).to eq(result)
end
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7