diff options
author | Mayra Cabrera <mcabrera@gitlab.com> | 2018-03-30 05:11:36 +0300 |
---|---|---|
committer | Mayra Cabrera <mcabrera@gitlab.com> | 2018-04-07 05:20:16 +0300 |
commit | 345ac03b7afb1dc9b941c53bc45cc3dfcf22e61c (patch) | |
tree | 42da609254928d746a961465d9ecfbff3ab71ad4 | |
parent | 370fc05da7f95bf6621867a71d51493cf3899e25 (diff) |
Address UX review
- Keep 'Deploy Section' open upon save, otherwise the token might get
lost
- When an error appears, display the error inside the form and also keep
the Deploy Section open
- Changue copy of revoke modal
12 files changed, 103 insertions, 50 deletions
diff --git a/app/controllers/projects/deploy_tokens_controller.rb b/app/controllers/projects/deploy_tokens_controller.rb index ecc6db50f2f..1b1bd461b27 100644 --- a/app/controllers/projects/deploy_tokens_controller.rb +++ b/app/controllers/projects/deploy_tokens_controller.rb @@ -3,16 +3,12 @@ class Projects::DeployTokensController < Projects::ApplicationController def create @token = DeployTokens::CreateService.new(@project, current_user, deploy_token_params).execute - token_params = {} if @token.valid? flash[:notice] = 'Your new project deploy token has been created.' - else - token_params = @token.attributes.slice("name", "scopes", "expires_at") - flash[:alert] = @token.errors.full_messages.join(', ').html_safe end - redirect_to project_settings_repository_path(project, deploy_token: token_params) + redirect_to project_settings_repository_path(project) end def revoke diff --git a/app/controllers/projects/settings/repository_controller.rb b/app/controllers/projects/settings/repository_controller.rb index 28897cc5946..ab6d8b3b10c 100644 --- a/app/controllers/projects/settings/repository_controller.rb +++ b/app/controllers/projects/settings/repository_controller.rb @@ -55,11 +55,9 @@ module Projects end def define_deploy_token - @deploy_token = @project.deploy_tokens.build(deploy_token_attributes) - end - - def deploy_token_attributes - params.fetch(:deploy_token, {}).permit(:name, :expires_at, scopes: []) + attributes = @deploy_tokens.attributes_deploy_token + @deploy_token = @project.deploy_tokens.build(attributes) + @deploy_token.valid? unless attributes.empty? end end end diff --git a/app/helpers/deploy_tokens_helper.rb b/app/helpers/deploy_tokens_helper.rb new file mode 100644 index 00000000000..68e1d56e951 --- /dev/null +++ b/app/helpers/deploy_tokens_helper.rb @@ -0,0 +1,7 @@ +module DeployTokensHelper + def expand_deploy_tokens_section?(temporal_token, deploy_token) + temporal_token.present? || + deploy_token.errors.present? || + Rails.env.test? + end +end diff --git a/app/models/deploy_token.rb b/app/models/deploy_token.rb index 089b4343f41..475ad06906a 100644 --- a/app/models/deploy_token.rb +++ b/app/models/deploy_token.rb @@ -19,8 +19,8 @@ class DeployToken < ActiveRecord::Base update!(revoked: true) end - def self.redis_shared_state_key(user_id) - "gitlab:personal_access_token:#{user_id}" + def redis_shared_state_key(user_id) + "gitlab:deploy_token:#{project_id}:#{user_id}" end def active? diff --git a/app/presenters/projects/settings/deploy_tokens_presenter.rb b/app/presenters/projects/settings/deploy_tokens_presenter.rb index cbad35431b3..bdb89bb9f02 100644 --- a/app/presenters/projects/settings/deploy_tokens_presenter.rb +++ b/app/presenters/projects/settings/deploy_tokens_presenter.rb @@ -23,14 +23,23 @@ module Projects end end - def new_deploy_token - @new_deploy_token ||= Gitlab::Redis::SharedState.with do |redis| + def temporal_token + @temporal_token ||= Gitlab::Redis::SharedState.with do |redis| token = redis.get(deploy_token_key) redis.del(deploy_token_key) token end end + def attributes_deploy_token + @attributes_deploy_token ||= Gitlab::Redis::SharedState.with do |redis| + attributes_key = deploy_token_key + ":attributes" + attributes_content = redis.get(attributes_key) || '{}' + redis.del(attributes_key) + JSON.parse(attributes_content) + end + end + private def scope_descriptions @@ -41,7 +50,7 @@ module Projects end def deploy_token_key - DeployToken.redis_shared_state_key(current_user.id) + @deploy_token_key ||= project.deploy_tokens.new.redis_shared_state_key(current_user.id) end end end diff --git a/app/services/deploy_tokens/create_service.rb b/app/services/deploy_tokens/create_service.rb index e93c021557e..0332bb54167 100644 --- a/app/services/deploy_tokens/create_service.rb +++ b/app/services/deploy_tokens/create_service.rb @@ -3,22 +3,36 @@ module DeployTokens REDIS_EXPIRY_TIME = 3.minutes def execute - @deploy_token = @project.deploy_tokens.create(params) - store_in_redis if @deploy_token.persisted? - - @deploy_token + @project.deploy_tokens.build.tap do |deploy_token| + deploy_token.attributes = params + deploy_token.save + store_deploy_token_info_in_redis(deploy_token) + end end private - def store_in_redis - Gitlab::Redis::SharedState.with do |redis| - redis.set(deploy_token_key, @deploy_token.token, ex: REDIS_EXPIRY_TIME) + def store_deploy_token_info_in_redis(deploy_token) + deploy_token_key = deploy_token.redis_shared_state_key(current_user.id) + + if deploy_token.persisted? + store_in_redis(deploy_token_key, deploy_token.token) + else + store_deploy_attributes(deploy_token_key, deploy_token) end end - def deploy_token_key - DeployToken.redis_shared_state_key(current_user.id) + def store_deploy_attributes(deploy_token_key, deploy_token) + attributes = deploy_token.attributes.slice("name", "expires_at") + deploy_token_attributes_key = deploy_token_key + ":attributes" + + store_in_redis(deploy_token_attributes_key, attributes.to_json) + end + + def store_in_redis(key, value) + Gitlab::Redis::SharedState.with do |redis| + redis.set(key, value, ex: REDIS_EXPIRY_TIME) + end end end end diff --git a/app/views/projects/deploy_tokens/_index.html.haml b/app/views/projects/deploy_tokens/_index.html.haml index 7d2bfe291c3..e1d80354f5c 100644 --- a/app/views/projects/deploy_tokens/_index.html.haml +++ b/app/views/projects/deploy_tokens/_index.html.haml @@ -1,4 +1,4 @@ -- expanded = Rails.env.test? +- expanded = expand_deploy_tokens_section?(@deploy_tokens.temporal_token, @deploy_token) %section.settings.no-animate{ class: ('expanded' if expanded) } .settings-header @@ -9,8 +9,8 @@ %p Deploy tokens allow read-only access to your repository and registry images. .settings-content - - if @deploy_tokens.new_deploy_token - = render 'projects/deploy_tokens/new_deploy_token', new_token: @deploy_tokens.new_deploy_token + - if @deploy_tokens.temporal_token + = render 'projects/deploy_tokens/new_deploy_token', new_token: @deploy_tokens.temporal_token %h5.prepend-top-0 Add a deploy token diff --git a/app/views/projects/deploy_tokens/_revoke_modal.html.haml b/app/views/projects/deploy_tokens/_revoke_modal.html.haml index a859aac015d..0700f0f7bd0 100644 --- a/app/views/projects/deploy_tokens/_revoke_modal.html.haml +++ b/app/views/projects/deploy_tokens/_revoke_modal.html.haml @@ -9,7 +9,9 @@ %span{ "aria-hidden" => "true" } × .modal-body %p - Are you sure you want to revoke this Deploy Token? This action cannot be undone + You are about to revoke + %b #{token.name}. + This action cannot be undone. .modal-footer %a{ href: '#', data: { dismiss: 'modal' }, class: 'btn btn-default' } Cancel = link_to "Revoke #{token.name}", revoke_project_deploy_token_path(project, token), method: :put, class: 'btn btn-danger' diff --git a/spec/controllers/projects/deploy_tokens_controller_spec.rb b/spec/controllers/projects/deploy_tokens_controller_spec.rb index 0ade61f4380..f037aacfe8e 100644 --- a/spec/controllers/projects/deploy_tokens_controller_spec.rb +++ b/spec/controllers/projects/deploy_tokens_controller_spec.rb @@ -27,6 +27,11 @@ describe Projects::DeployTokensController do subject expect(flash[:notice]).to eq('Your new project deploy token has been created.') end + + it 'should redirect to project settings repository' do + subject + expect(response).to redirect_to project_settings_repository_path(project) + end end context 'with invalid params' do @@ -36,9 +41,9 @@ describe Projects::DeployTokensController do expect { subject }.not_to change(DeployToken, :count) end - it 'should include a flash alert with the error message' do + it 'should redirect to project settings repository' do subject - expect(flash[:alert]).to eq("Scopes can't be blank") + expect(response).to redirect_to project_settings_repository_path(project) end end diff --git a/spec/controllers/projects/settings/repository_controller_spec.rb b/spec/controllers/projects/settings/repository_controller_spec.rb index 03867661483..da4d3e5732d 100644 --- a/spec/controllers/projects/settings/repository_controller_spec.rb +++ b/spec/controllers/projects/settings/repository_controller_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Projects::Settings::RepositoryController do +describe Projects::Settings::RepositoryController, :clean_gitlab_redis_shared_state do let(:project) { create(:project_empty_repo, :public) } let(:user) { create(:user) } @@ -17,7 +17,7 @@ describe Projects::Settings::RepositoryController do expect(response).to render_template(:show) end - context 'with no deploy token params' do + context 'with no deploy token attributes present' do it 'should build an empty instance of DeployToken' do get :show, namespace_id: project.namespace, project_id: project @@ -29,17 +29,29 @@ describe Projects::Settings::RepositoryController do end end - context 'when rendering an invalid deploy token' do - let(:deploy_token_attributes) { attributes_for(:deploy_token, project_id: project.id) } + context 'with deploy token attributes present' do + let(:deploy_token_key) { "gitlab:deploy_token:#{project.id}:#{user.id}:attributes" } - it 'should build an instance of DeployToken' do - get :show, namespace_id: project.namespace, project_id: project, deploy_token: deploy_token_attributes + let(:deploy_token_attributes) do + { + name: 'test-token', + expires_at: Date.today + 1.month + } + end + before do + Gitlab::Redis::SharedState.with do |redis| + redis.set(deploy_token_key, deploy_token_attributes.to_json) + end + + get :show, namespace_id: project.namespace, project_id: project + end + + it 'should build an instance of DeployToken' do deploy_token = assigns(:deploy_token) expect(deploy_token).to be_an_instance_of(DeployToken) expect(deploy_token.name).to eq(deploy_token_attributes[:name]) expect(deploy_token.expires_at.to_date).to eq(deploy_token_attributes[:expires_at].to_date) - expect(deploy_token.scopes).to match_array(deploy_token_attributes[:scopes]) end end end diff --git a/spec/features/projects/settings/repository_settings_spec.rb b/spec/features/projects/settings/repository_settings_spec.rb index 1106a31d444..f0997b6809d 100644 --- a/spec/features/projects/settings/repository_settings_spec.rb +++ b/spec/features/projects/settings/repository_settings_spec.rb @@ -90,17 +90,16 @@ feature 'Repository settings' do end context 'Deploy tokens' do - let(:deploy_token) { create(:deploy_token, project: project, expires_at: Date.today + 2.days) } + let(:deploy_token) { create(:deploy_token, project: project) } before do project.deploy_tokens << deploy_token visit project_settings_repository_path(project) - end + end scenario 'view deploy tokens' do within('.deploy-tokens') do expect(page).to have_content(deploy_token.name) - expect(page).to have_content('In 1 day') expect(page).to have_content(deploy_token.scopes.join(", ")) end end @@ -121,7 +120,6 @@ feature 'Repository settings' do click_link "Revoke #{deploy_token.name}" expect(page).not_to have_content(deploy_token.name) - expect(page).not_to have_content('In 1 day') expect(page).not_to have_content(deploy_token.scopes.join(", ")) end end diff --git a/spec/services/deploy_tokens/create_service_spec.rb b/spec/services/deploy_tokens/create_service_spec.rb index 84aa17971d6..df18213cf84 100644 --- a/spec/services/deploy_tokens/create_service_spec.rb +++ b/spec/services/deploy_tokens/create_service_spec.rb @@ -6,40 +6,52 @@ describe DeployTokens::CreateService, :clean_gitlab_redis_shared_state do let(:deploy_token_params) { attributes_for(:deploy_token) } describe '#execute' do - subject { described_class.new(project, user, deploy_token_params) } + subject { described_class.new(project, user, deploy_token_params).execute } context 'when the deploy token is valid' do it 'should create a new DeployToken' do - expect { subject.execute }.to change { DeployToken.count }.by(1) + expect { subject }.to change { DeployToken.count }.by(1) end - it 'should assign the DeployToken to the project' do - subject.execute + it 'returns a DeployToken' do + expect(subject).to be_an_instance_of DeployToken + end + it 'should assign the DeployToken to the project' do expect(subject.project).to eq(project) end it 'should store the token on redis' do - subject.execute - redis_key = DeployToken.redis_shared_state_key(user.id) + redis_key = subject.redis_shared_state_key(user.id) expect(Gitlab::Redis::SharedState.with { |redis| redis.get(redis_key) }).not_to be_nil end + + it 'should not store deploy token attributes on redis' do + redis_key = subject.redis_shared_state_key(user.id) + ":attributes" + + expect(Gitlab::Redis::SharedState.with { |redis| redis.get(redis_key) }).to be_nil + end end context 'when the deploy token is invalid' do let(:deploy_token_params) { attributes_for(:deploy_token, scopes: []) } it 'it should not create a new DeployToken' do - expect { subject.execute }.not_to change { DeployToken.count } + expect { subject }.not_to change { DeployToken.count } end it 'should not store the token on redis' do - subject.execute - redis_key = DeployToken.redis_shared_state_key(user.id) + redis_key = subject.redis_shared_state_key(user.id) expect(Gitlab::Redis::SharedState.with { |redis| redis.get(redis_key) }).to be_nil end + + it 'should store deploy token attributes on redis' do + redis_key = subject.redis_shared_state_key(user.id) + ":attributes" + + expect(Gitlab::Redis::SharedState.with { |redis| redis.get(redis_key) }).not_to be_nil + end end end end |