Improve error message when providing an invalid deploy key

Use a single succinct error message when a deploy key is invalid
instead of returning all validation errors.

Changelog: fixed

.rubocop_todo/style/format_string.yml

@@ -142,6 +142,7 @@ Style/FormatString:
     - 'app/presenters/ci/pipeline_presenter.rb'
     - 'app/presenters/merge_request_presenter.rb'
     - 'app/presenters/project_presenter.rb'
+    - 'app/presenters/key_presenter.rb'
     - 'app/serializers/build_details_entity.rb'
     - 'app/services/alert_management/alerts/update_service.rb'
     - 'app/services/boards/lists/base_create_service.rb'

app/controllers/projects/deploy_keys_controller.rb

@@ -27,11 +27,9 @@ def new
   end
 
   def create
-    @key = DeployKeys::CreateService.new(current_user, create_params).execute(project: @project)
+    @key = DeployKeys::CreateService.new(current_user, create_params).execute(project: @project).present
 
-    unless @key.valid?
-      flash[:alert] = @key.errors.full_messages.join(', ').html_safe
-    end
+    flash[:alert] = @key.humanized_error_message unless @key.valid?
 
     redirect_to_repository
   end

app/models/deploy_key.rb

@@ -4,6 +4,7 @@ class DeployKey < Key
   include FromUnion
   include IgnorableColumns
   include PolicyActor
+  include Presentable
 
   has_many :deploy_keys_projects, inverse_of: :deploy_key, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :projects, through: :deploy_keys_projects

app/presenters/deploy_key_presenter.rb

+# frozen_string_literal: true
+
+class DeployKeyPresenter < KeyPresenter # rubocop:disable Gitlab/NamespacedClass
+  presents ::DeployKey, as: :deploy_key
+
+  def humanized_error_message
+    super(type: :deploy_key)
+  end
+end

app/presenters/key_presenter.rb

+# frozen_string_literal: true
+
+class KeyPresenter < Gitlab::View::Presenter::Delegated # rubocop:disable Gitlab/NamespacedClass
+  presents ::Key, as: :key_object
+
+  def humanized_error_message(type: :key)
+    if !key_object.public_key.valid?
+      help_link = help_page_link(_('supported SSH public key.'), 'user/ssh', 'supported-ssh-key-types')
+
+      _('%{type} must be a %{help_link}').html_safe % { type: type.to_s.titleize, help_link: help_link }
+    else
+      key_object.errors.full_messages.join(', ').html_safe
+    end
+  end
+
+  private
+
+  def help_page_link(title, path, anchor)
+    ActionController::Base.helpers.link_to(title, help_page_path(path, anchor: anchor),
+      target: '_blank', rel: 'noopener noreferrer')
+  end
+end

locale/gitlab.pot

@@ -1134,6 +1134,9 @@ msgstr ""
 msgid "%{total} warnings found: showing first %{warningsDisplayed}"
 msgstr ""
+msgid "%{type} must be a %{help_link}"
+msgstr ""
+
 msgid "%{type} only supports %{name} name"
 msgstr ""
@@ -48567,6 +48570,9 @@ msgstr ""
 msgid "suggestPipeline|We’re adding a GitLab CI configuration file to add a pipeline to the project. You could create it manually, but we recommend that you start with a GitLab template that works out of the box."
 msgstr ""
+msgid "supported SSH public key."
+msgstr ""
+
 msgid "tag name"
 msgstr ""

spec/controllers/projects/deploy_keys_controller_spec.rb

@@ -72,13 +72,15 @@
   end
 
   describe 'POST create' do
+    let(:deploy_key_content) { attributes_for(:deploy_key)[:key] }
+
     def create_params(title = 'my-key')
       {
         namespace_id: project.namespace.path,
         project_id: project.path,
         deploy_key: {
           title: title,
-          key: attributes_for(:deploy_key)[:key],
+          key: deploy_key_content,
           deploy_keys_projects_attributes: { '0' => { can_push: '1' } }
         }
       }
@@ -96,13 +98,38 @@ def create_params(title = 'my-key')
       expect(response).to redirect_to(project_settings_repository_path(project, anchor: 'js-deploy-keys-settings'))
     end
 
-    context 'when the deploy key is invalid' do
+    context 'when the deploy key has an invalid title' do
       it 'shows an alert with the validations errors' do
         post :create, params: create_params(nil)
 
         expect(flash[:alert]).to eq("Title can't be blank, Deploy keys projects deploy key title can't be blank")
       end
     end
+
+    context 'when the deploy key is not supported SSH public key' do
+      let(:deploy_key_content) { 'bogus ssh public key' }
+
+      it 'shows an alert with a help link' do
+        post :create, params: create_params
+
+        expect(assigns(:key).errors.count).to be > 1
+        expect(flash[:alert]).to eq('Deploy Key must be a <a target="_blank" rel="noopener noreferrer" ' \
+          'href="/help/user/ssh#supported-ssh-key-types">supported SSH public key.</a>')
+      end
+    end
+
+    context 'when the deploy key already exists' do
+      before do
+        create(:deploy_key, title: 'my-key', key: deploy_key_content, projects: [project])
+      end
+
+      it 'shows an alert with the validations errors' do
+        post :create, params: create_params
+
+        expect(flash[:alert]).to eq("Fingerprint sha256 has already been taken, " \
+          "Deploy keys projects deploy key fingerprint sha256 has already been taken")
+      end
+    end
   end
 
   describe '/enable/:id' do

spec/presenters/deploy_key_presenter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe DeployKeyPresenter do
+  let(:presenter) { described_class.new(deploy_key) }
+
+  describe '#humanized_error_message' do
+    subject { presenter.humanized_error_message }
+
+    before do
+      deploy_key.valid?
+    end
+
+    context 'when public key is unsupported' do
+      let(:deploy_key) { build(:deploy_key, key: 'a') }
+
+      it 'returns the custom error message' do
+        expect(subject).to eq('Deploy Key must be a <a target="_blank" rel="noopener noreferrer" ' \
+          'href="/help/user/ssh#supported-ssh-key-types">supported SSH public key.</a>')
+      end
+    end
+  end
+end

spec/presenters/key_presenter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe KeyPresenter do
+  let(:presenter) { described_class.new(key) }
+
+  describe '#humanized_error_message' do
+    subject { presenter.humanized_error_message }
+
+    before do
+      key.valid?
+    end
+
+    context 'when public key is unsupported' do
+      let(:key) { build(:key, key: 'a') }
+
+      it 'returns the custom error message' do
+        expect(subject).to eq('Key must be a <a target="_blank" rel="noopener noreferrer" ' \
+          'href="/help/user/ssh#supported-ssh-key-types">supported SSH public key.</a>')
+      end
+    end
+
+    context 'when key is expired' do
+      let(:key) { build(:key, :expired) }
+
+      it 'returns Active Record error message' do
+        expect(subject).to eq('Key has expired')
+      end
+    end
+  end
+end