diff --git a/app/assets/stylesheets/shared/general-bootstrap.sass b/app/assets/stylesheets/shared/general-bootstrap.sass index 29a605cb7..20c8c3336 100644 --- a/app/assets/stylesheets/shared/general-bootstrap.sass +++ b/app/assets/stylesheets/shared/general-bootstrap.sass @@ -65,3 +65,6 @@ .required:after content: "*" margin: 0 0 0 1px + +.not-required:after + content: '' diff --git a/app/assets/stylesheets/shared/general.sass b/app/assets/stylesheets/shared/general.sass index b0b11c9aa..1a2975b5f 100644 --- a/app/assets/stylesheets/shared/general.sass +++ b/app/assets/stylesheets/shared/general.sass @@ -57,3 +57,5 @@ body > .container .text-grey color: grey + + diff --git a/app/controllers/admin/admin_users_controller.rb b/app/controllers/admin/admin_users_controller.rb index 2a0d24689..eb4a4cb25 100644 --- a/app/controllers/admin/admin_users_controller.rb +++ b/app/controllers/admin/admin_users_controller.rb @@ -11,6 +11,10 @@ class Admin::AdminUsersController < AdminController @admin_user = AdminUser.new end + def show; end + + def edit; end + def create @admin_user = AdminUser.new(admin_user_params) @@ -23,12 +27,11 @@ class Admin::AdminUsersController < AdminController end end - def show; end - - def edit; end - def update - if @admin_user.update(admin_user_params) + params[:admin_user].delete(:password) if params[:admin_user][:password].blank? + params[:admin_user].delete(:password_confirmation) if params[:admin_user][:password_confirmation].blank? + + if @admin_user.update_attributes(admin_user_params) flash[:notice] = I18n.t('record_updated') redirect_to [:admin, @admin_user] else diff --git a/app/controllers/admin/api_users_controller.rb b/app/controllers/admin/api_users_controller.rb index be2170016..6e0dd7f0f 100644 --- a/app/controllers/admin/api_users_controller.rb +++ b/app/controllers/admin/api_users_controller.rb @@ -29,6 +29,7 @@ class Admin::ApiUsersController < AdminController def edit; end def update + params[:api_user].delete(:password) if params[:api_user][:password].blank? if @api_user.update(api_user_params) flash[:notice] = I18n.t('record_updated') redirect_to [:admin, @api_user] diff --git a/app/models/admin_user.rb b/app/models/admin_user.rb index 79e8f5649..0634baef4 100644 --- a/app/models/admin_user.rb +++ b/app/models/admin_user.rb @@ -1,9 +1,10 @@ class AdminUser < User - validates :username, :password, :country_code, :roles, presence: true + validates :username, :country_code, :roles, presence: true validates :identity_code, uniqueness: true, allow_blank: true validates :identity_code, presence: true, if: -> { country_code == 'EE' } validates :email, presence: true - + validates :password, :password_confirmation, presence: true, if: :new_record? + validates :password_confirmation, presence: true, if: :encrypted_password_changed? validate :validate_identity_code, if: -> { country_code == 'EE' } ROLES = %w(user customer_service admin) # should not match to api_users roles diff --git a/app/views/admin/admin_users/_form.haml b/app/views/admin/admin_users/_form.haml index 8db7583f2..9a9aa8a80 100644 --- a/app/views/admin/admin_users/_form.haml +++ b/app/views/admin/admin_users/_form.haml @@ -11,14 +11,15 @@ - if @admin_user.new_record? || can?(:update, AdminUser) .form-group .col-md-4.control-label - = f.label :password + - not_required = @admin_user.new_record? ? '' : 'not-required' + = f.label :password, class: not_required .col-md-8 - = f.text_field(:password, class: 'form-control') + = f.password_field(:password, class: 'form-control') .form-group .col-md-4.control-label - = f.label :password_confirmation + = f.label :password_confirmation, class: not_required .col-md-8 - = f.text_field(:password_confirmation, class: 'form-control') + = f.password_field(:password_confirmation, class: 'form-control') %hr .form-group @@ -48,3 +49,8 @@ .row .col-md-8.text-right = button_tag(t(:save), class: 'btn btn-primary') + +:coffee + $("#admin_user_password").removeAttr('required') + $("#admin_user_password_confirmation").removeAttr('required') + diff --git a/app/views/admin/api_users/_form.haml b/app/views/admin/api_users/_form.haml index 24ca8465b..e6851e424 100644 --- a/app/views/admin/api_users/_form.haml +++ b/app/views/admin/api_users/_form.haml @@ -1,4 +1,5 @@ -= form_for([:admin, @api_user], multipart: true, html: {class: 'form-horizontal'}) do |f| += form_for([:admin, @api_user], multipart: true, + html: {class: 'form-horizontal', autocomplete: 'off'}) do |f| = render 'shared/full_errors', object: @api_user .row @@ -10,9 +11,11 @@ = f.text_field(:username, class: 'form-control') .form-group .col-md-4.control-label - = f.label :password + - not_required = @api_user.new_record? ? '' : 'not-required' + = f.label :password, class: not_required .col-md-7 - = f.text_field(:password, class: 'form-control') + = f.text_field :password, class: 'form-control', autocomplete: 'off' + .form-group .col-md-4.control-label = f.label :identity_code @@ -48,4 +51,5 @@ = button_tag(t(:save), class: 'btn btn-primary') :coffee - Autocomplete.bindAdminRegistrarSearch(); + Autocomplete.bindAdminRegistrarSearch() + $("#api_user_password").removeAttr('required') diff --git a/spec/fabricators/admin_user_fabricator.rb b/spec/fabricators/admin_user_fabricator.rb index 2c54ad938..a73596810 100644 --- a/spec/fabricators/admin_user_fabricator.rb +++ b/spec/fabricators/admin_user_fabricator.rb @@ -4,7 +4,7 @@ Fabricator(:admin_user) do username 'gitlab' password 'ghyt9e4fu' password_confirmation 'ghyt9e4fu' - email 'info@gitlab.eu' + email { sequence(:email) { |i| "info#{i}@example.com" } } country_code 'FI' roles ['admin'] end diff --git a/spec/models/admin_user_spec.rb b/spec/models/admin_user_spec.rb index d24ca24c5..a6b668a5d 100644 --- a/spec/models/admin_user_spec.rb +++ b/spec/models/admin_user_spec.rb @@ -4,95 +4,61 @@ require 'cancan/matchers' describe AdminUser do context 'with invalid attribute' do before :all do - @user = AdminUser.new + @admin_user = AdminUser.new end it 'should not be valid' do - @user.valid? - @user.errors.full_messages.should match_array([ + @admin_user.valid? + @admin_user.errors.full_messages.should match_array([ "Country code is missing", "Email Email is missing", "Email Email is missing", "Password Password is missing", "Password Password is missing", + "Password confirmation is missing", "Roles is missing", "Username Username is missing" ]) end it 'should not have any versions' do - @user.versions.should == [] + @admin_user.versions.should == [] end end context 'with valid attributes' do before :all do - @user = Fabricate(:admin_user) + @admin_user = Fabricate(:admin_user) end it 'should be valid' do - @user.valid? - @user.errors.full_messages.should match_array([]) + @admin_user.valid? + @admin_user.errors.full_messages.should match_array([]) end - # it 'should be valid twice' do - # @user = Fabricate(:admin_user) - # @user.valid? - # @user.errors.full_messages.should match_array([]) - # end + it 'should be valid twice' do + @admin_user = Fabricate(:admin_user) + @admin_user.valid? + @admin_user.errors.full_messages.should match_array([]) + end - # it 'should have one version' do - # with_versioning do - # @user.versions.should == [] - # @user.zip = 'New zip' - # @user.save - # @user.errors.full_messages.should match_array([]) - # @user.versions.size.should == 1 - # end - # end + it 'should have one version' do + with_versioning do + @admin_user.versions.should == [] + @admin_user.updated_at = Time.zone.now + @admin_user.save + @admin_user.errors.full_messages.should match_array([]) + @admin_user.versions.size.should == 1 + end + end + + it 'should require password confirmation when changing password' do + @admin_user.valid?.should == true + @admin_user.password = 'not confirmed' + @admin_user.valid? + @admin_user.errors.full_messages.should match_array([ + "Password confirmation doesn't match Password" + ]) + end end - - # describe 'abilities' do - # subject(:ability) { Ability.new(user) } - # let(:user) { nil } - - # context 'when user is admin' do - # let(:user) { Fabricate(:admin_user) } - - # it { should be_able_to(:manage, Domain.new) } - # it { should be_able_to(:manage, Contact.new) } - # it { should be_able_to(:manage, Registrar.new) } - # it { should be_able_to(:manage, Setting.new) } - # it { should be_able_to(:manage, ZonefileSetting.new) } - # it { should be_able_to(:manage, DomainVersion.new) } - # it { should be_able_to(:manage, User.new) } - # it { should be_able_to(:manage, ApiUser.new) } - # it { should be_able_to(:manage, Keyrelay.new) } - # it { should be_able_to(:manage, LegalDocument.new) } - # it { should be_able_to(:read, ApiLog::EppLog.new) } - # it { should be_able_to(:read, ApiLog::ReppLog.new) } - # it { should be_able_to(:index, :delayed_job) } - # it { should be_able_to(:create, :zonefile) } - # it { should be_able_to(:access, :settings_menu) } - # end - - # context 'when user is customer service' do - # let(:user) { Fabricate(:user, roles: ['customer_service']) } - - # it { should be_able_to(:manage, Domain.new) } - # it { should be_able_to(:manage, Contact.new) } - # it { should be_able_to(:manage, Registrar.new) } - # it { should_not be_able_to(:manage, Setting.new) } - # it { should_not be_able_to(:manage, ZonefileSetting.new) } - # it { should_not be_able_to(:manage, DomainVersion.new) } - # it { should_not be_able_to(:manage, User.new) } - # it { should_not be_able_to(:manage, ApiUser.new) } - # it { should_not be_able_to(:manage, LegalDocument.new) } - # it { should_not be_able_to(:read, ApiLog::EppLog.new) } - # it { should_not be_able_to(:read, ApiLog::ReppLog.new) } - # it { should_not be_able_to(:index, :delayed_job) } - # it { should_not be_able_to(:create, :zonefile) } - # it { should_not be_able_to(:access, :settings_menu) } - # end - # end end