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