Introduce a field DSL for integrations

This aims to improve the developer experience for integrations.

Currently for every accessible field, we need to define prop
accessors and field definitions. This change introduces a couple of
class-level DSL methods to make this clearer, less error prone and less
duplicative, with sensible defaults in some cases.

A Field class is introduced to make transformations of this data easier.

app/models/concerns/integrations/has_issue_tracker_fields.rb

+# frozen_string_literal: true
+
+module Integrations
+  module HasIssueTrackerFields
+    extend ActiveSupport::Concern
+
+    included do
+      field :project_url,
+            required: true,
+            storage: :data_fields,
+            title: -> { _('Project URL') },
+            help: -> { s_('IssueTracker|The URL to the project in the external issue tracker.') }
+
+      field :issues_url,
+            required: true,
+            storage: :data_fields,
+            title: -> { s_('IssueTracker|Issue URL') },
+            help: -> do
+              format s_('IssueTracker|The URL to view an issue in the external issue tracker. Must contain %{colon_id}.'),
+                colon_id: '<code>:id</code>'.html_safe
+            end
+
+      field :new_issue_url,
+            required: true,
+            storage: :data_fields,
+            title: -> { s_('IssueTracker|New issue URL') },
+            help: -> { s_('IssueTracker|The URL to create an issue in the external issue tracker.') }
+    end
+  end
+end

app/models/integration.rb

@@ -122,6 +122,39 @@ class Integration < ApplicationRecord
   scope :alert_hooks, -> { where(alert_events: true, active: true) }
   scope :deployment, -> { where(category: 'deployment') }
 
+  class << self
+    private
+
+    attr_writer :field_storage
+
+    def field_storage
+      @field_storage || :properties
+    end
+  end
+
+  # :nocov: Tested on subclasses.
+  def self.field(name, storage: field_storage, **attrs)
+    fields << ::Integrations::Field.new(name: name, **attrs)
+
+    case storage
+    when :properties
+      prop_accessor(name)
+    when :data_fields
+      data_field(name)
+    else
+      raise ArgumentError, "Unknown field storage: #{storage}"
+    end
+  end
+  # :nocov:
+
+  def self.fields
+    @fields ||= []
+  end
+
+  def fields
+    self.class.fields
+  end
+
   # Provide convenient accessor methods for each serialized property.
   # Also keep track of updated properties in a similar way as ActiveModel::Dirty
   def self.prop_accessor(*args)
@@ -395,11 +428,6 @@ class Integration < ApplicationRecord
     self.class.to_param
   end
 
-  def fields
-    # implement inside child
-    []
-  end
-
   def sections
     []
   end

app/models/integrations/base_issue_tracker.rb

@@ -4,10 +4,6 @@ module Integrations
   class BaseIssueTracker < Integration
     validate :one_issue_tracker, if: :activated?, on: :manual_change
 
-    # TODO: we can probably just delegate as part of
-    # https://gitlab.com/gitlab-org/gitlab/issues/29404
-    data_field :project_url, :issues_url, :new_issue_url
-
     default_value_for :category, 'issue_tracker'
 
     before_validation :handle_properties
@@ -72,14 +68,6 @@ module Integrations
       issue_url(iid)
     end
 
-    def fields
-      [
-        { type: 'text', name: 'project_url', title: _('Project URL'), help: s_('IssueTracker|The URL to the project in the external issue tracker.'), required: true },
-        { type: 'text', name: 'issues_url', title: s_('IssueTracker|Issue URL'), help: s_('IssueTracker|The URL to view an issue in the external issue tracker. Must contain %{colon_id}.') % { colon_id: '<code>:id</code>'.html_safe }, required: true },
-        { type: 'text', name: 'new_issue_url', title: s_('IssueTracker|New issue URL'), help: s_('IssueTracker|The URL to create an issue in the external issue tracker.'), required: true }
-      ]
-    end
-
     def initialize_properties
       {}
     end

app/models/integrations/bugzilla.rb

@@ -2,6 +2,8 @@
 
 module Integrations
   class Bugzilla < BaseIssueTracker
+    include Integrations::HasIssueTrackerFields
+
     validates :project_url, :issues_url, :new_issue_url, presence: true, public_url: true, if: :activated?
 
     def title

app/models/integrations/custom_issue_tracker.rb

@@ -2,6 +2,8 @@
 
 module Integrations
   class CustomIssueTracker < BaseIssueTracker
+    include HasIssueTrackerFields
+
     validates :project_url, :issues_url, :new_issue_url, presence: true, public_url: true, if: :activated?
 
     def title

app/models/integrations/ewm.rb

@@ -2,6 +2,8 @@
 
 module Integrations
   class Ewm < BaseIssueTracker
+    include HasIssueTrackerFields
+
     validates :project_url, :issues_url, :new_issue_url, presence: true, public_url: true, if: :activated?
 
     def self.reference_pattern(only_long: true)

app/models/integrations/field.rb

+# frozen_string_literal: true
+
+module Integrations
+  class Field
+    SENSITIVE_NAME = %r/token|key|password|passphrase|secret/.freeze
+
+    ATTRIBUTES = %i[
+      section type placeholder required choices value checkbox_label
+      title help
+      non_empty_password_help
+      non_empty_password_title
+      api_only
+    ].freeze
+
+    attr_reader :name
+
+    def initialize(name:, type: 'text', api_only: false, **attributes)
+      @name = name.to_s.freeze
+
+      attributes[:type] = SENSITIVE_NAME.match?(@name) ? 'password' : type
+      attributes[:api_only] = api_only
+      @attributes = attributes.freeze
+    end
+
+    def [](key)
+      return name if key == :name
+
+      value = @attributes[key]
+      return value.call if value.respond_to?(:call)
+
+      value
+    end
+
+    def sensitive?
+      @attributes[:type] == 'password'
+    end
+
+    ATTRIBUTES.each do |name|
+      define_method(name) { self[name] }
+    end
+  end
+end

app/models/integrations/jira.rb

@@ -28,11 +28,6 @@ module Integrations
     # We should use username/password for Jira Server and email/api_token for Jira Cloud,
     # for more information check: https://gitlab.com/gitlab-org/gitlab-foss/issues/49936.
 
-    # TODO: we can probably just delegate as part of
-    # https://gitlab.com/gitlab-org/gitlab/issues/29404
-    data_field :username, :password, :url, :api_url, :jira_issue_transition_automatic, :jira_issue_transition_id, :project_key, :issues_enabled,
-      :vulnerabilities_enabled, :vulnerabilities_issuetype
-
     before_validation :reset_password
     after_commit :update_deployment_type, on: [:create, :update], if: :update_deployment_type?
 
@@ -41,6 +36,44 @@ module Integrations
       all_details: 2
     }
 
+    self.field_storage = :data_fields
+
+    field :url,
+          section: SECTION_TYPE_CONNECTION,
+          required: true,
+          title: -> { s_('JiraService|Web URL') },
+          help: -> { s_('JiraService|Base URL of the Jira instance.') },
+          placeholder: 'https://jira.example.com'
+
+    field :api_url,
+          section: SECTION_TYPE_CONNECTION,
+          title: -> { s_('JiraService|Jira API URL') },
+          help: -> { s_('JiraService|If different from Web URL.') }
+
+    field :username,
+          section: SECTION_TYPE_CONNECTION,
+          required: true,
+          title: -> { s_('JiraService|Username or Email') },
+          help: -> { s_('JiraService|Use a username for server version and an email for cloud version.') }
+
+    field :password,
+          section: SECTION_TYPE_CONNECTION,
+          required: true,
+          title: -> { s_('JiraService|Password or API token') },
+          non_empty_password_title: -> { s_('JiraService|Enter new password or API token') },
+          non_empty_password_help: -> { s_('JiraService|Leave blank to use your current password or API token.') },
+          help: -> { s_('JiraService|Use a password for server version and an API token for cloud version.') }
+
+    # TODO: we can probably just delegate as part of
+    # https://gitlab.com/gitlab-org/gitlab/issues/29404
+    # These fields are API only, so no field definition is required.
+    data_field :jira_issue_transition_automatic
+    data_field :jira_issue_transition_id
+    data_field :project_key
+    data_field :issues_enabled
+    data_field :vulnerabilities_enabled
+    data_field :vulnerabilities_issuetype
+
     # When these are false GitLab does not create cross reference
     # comments on Jira except when an issue gets transitioned.
     def self.supported_events
@@ -127,45 +160,6 @@ module Integrations
       'jira'
     end
 
-    def fields
-      [
-        {
-          section: SECTION_TYPE_CONNECTION,
-          type: 'text',
-          name: 'url',
-          title: s_('JiraService|Web URL'),
-          placeholder: 'https://jira.example.com',
-          help: s_('JiraService|Base URL of the Jira instance.'),
-          required: true
-        },
-        {
-          section: SECTION_TYPE_CONNECTION,
-          type: 'text',
-          name: 'api_url',
-          title: s_('JiraService|Jira API URL'),
-          help: s_('JiraService|If different from Web URL.')
-        },
-        {
-          section: SECTION_TYPE_CONNECTION,
-          type: 'text',
-          name: 'username',
-          title: s_('JiraService|Username or Email'),
-          help: s_('JiraService|Use a username for server version and an email for cloud version.'),
-          required: true
-        },
-        {
-          section: SECTION_TYPE_CONNECTION,
-          type: 'password',
-          name: 'password',
-          title: s_('JiraService|Password or API token'),
-          non_empty_password_title: s_('JiraService|Enter new password or API token'),
-          non_empty_password_help: s_('JiraService|Leave blank to use your current password or API token.'),
-          help: s_('JiraService|Use a password for server version and an API token for cloud version.'),
-          required: true
-        }
-      ]
-    end
-
     def sections
       [
         {
@@ -194,17 +188,12 @@ module Integrations
       url.to_s
     end
 
-    override :project_url
-    def project_url
-      web_url
-    end
+    alias_method :project_url, :web_url
 
-    override :issues_url
     def issues_url
       web_url('browse/:id')
     end
 
-    override :new_issue_url
     def new_issue_url
       web_url('secure/CreateIssue!default.jspa')
     end

app/models/integrations/redmine.rb

@@ -2,6 +2,8 @@
 
 module Integrations
   class Redmine < BaseIssueTracker
+    include Integrations::HasIssueTrackerFields
+
     validates :project_url, :issues_url, :new_issue_url, presence: true, public_url: true, if: :activated?
 
     def title

app/models/integrations/youtrack.rb

@@ -2,6 +2,8 @@
 
 module Integrations
   class Youtrack < BaseIssueTracker
+    include Integrations::HasIssueTrackerFields
+
     validates :project_url, :issues_url, presence: true, public_url: true, if: :activated?
 
     # {PROJECT-KEY}-{NUMBER} Examples: YT-1, PRJ-1, gl-030

spec/models/integration_spec.rb

@@ -719,34 +719,63 @@ RSpec.describe Integration do
   end
 
   describe '#api_field_names' do
-    let(:fake_integration) do
-      Class.new(Integration) do
-        def fields
-          [
-            { name: 'token' },
-            { name: 'api_token' },
-            { name: 'token_api' },
-            { name: 'safe_token' },
-            { name: 'key' },
-            { name: 'api_key' },
-            { name: 'password' },
-            { name: 'password_field' },
-            { name: 'some_safe_field' },
-            { name: 'safe_field' },
-            { name: 'url' },
-            { name: 'trojan_horse', type: 'password' },
-            { name: 'trojan_gift', type: 'gift' }
-          ].shuffle
-        end
+    shared_examples 'api field names' do
+      it 'filters out sensitive fields' do
+        safe_fields = %w[some_safe_field safe_field url trojan_gift]
+
+        expect(fake_integration.new).to have_attributes(
+          api_field_names: match_array(safe_fields)
+        )
       end
     end
 
-    it 'filters out sensitive fields' do
-      safe_fields = %w[some_safe_field safe_field url trojan_gift]
+    context 'when the class overrides #fields' do
+      let(:fake_integration) do
+        Class.new(Integration) do
+          def fields
+            [
+              { name: 'token' },
+              { name: 'api_token' },
+              { name: 'token_api' },
+              { name: 'safe_token' },
+              { name: 'key' },
+              { name: 'api_key' },
+              { name: 'password' },
+              { name: 'password_field' },
+              { name: 'some_safe_field' },
+              { name: 'safe_field' },
+              { name: 'url' },
+              { name: 'trojan_horse', type: 'password' },
+              { name: 'trojan_gift', type: 'gift' }
+            ].shuffle
+          end
+        end
+      end
 
-      expect(fake_integration.new).to have_attributes(
-        api_field_names: match_array(safe_fields)
-      )
+      it_behaves_like 'api field names'
+    end
+
+    context 'when the class uses the field DSL' do
+      let(:fake_integration) do
+        Class.new(described_class) do
+          field :token
+          field :token
+          field :api_token
+          field :token_api
+          field :safe_token
+          field :key
+          field :api_key
+          field :password
+          field :password_field
+          field :some_safe_field
+          field :safe_field
+          field :url
+          field :trojan_horse, type: 'password'
+          field :trojan_gift, type: 'gift'
+        end
+      end
+
+      it_behaves_like 'api field names'
     end
   end
 
@@ -921,4 +950,75 @@ RSpec.describe Integration do
       end
     end
   end
+
+  describe 'field DSL' do
+    let(:integration_type) do
+      Class.new(described_class) do
+        field :foo
+        field :foo_p, storage: :properties
+        field :foo_dt, storage: :data_fields
+
+        field :bar, type: 'password'
+        field :password
+
+        field :with_help,
+              help: -> { 'help' }
+
+        field :a_number,
+              type: 'number'
+      end
+    end
+
+    before do
+      allow(integration).to receive(:data_fields).and_return(data_fields)
+    end
+
+    let(:integration) { integration_type.new }
+    let(:data_fields) { Struct.new(:foo_dt).new }
+
+    it 'checks the value of storage' do
+      expect do
+        Class.new(described_class) { field(:foo, storage: 'bar') }
+      end.to raise_error(ArgumentError, /Unknown field storage/)
+    end
+
+    it 'provides prop_accessors' do
+      integration.foo = 1
+      expect(integration.foo).to eq 1
+      expect(integration.properties['foo']).to eq 1
+      expect(integration).to be_foo_changed
+
+      integration.foo_p = 2
+      expect(integration.foo_p).to eq 2
+      expect(integration.properties['foo_p']).to eq 2
+      expect(integration).to be_foo_p_changed
+    end
+
+    it 'provides data fields' do
+      integration.foo_dt = 3
+      expect(integration.foo_dt).to eq 3
+      expect(data_fields.foo_dt).to eq 3
+      expect(integration).to be_foo_dt_changed
+    end
+
+    it 'registers fields in the fields list' do
+      expect(integration.fields.pluck(:name)).to match_array %w[
+        foo foo_p foo_dt bar password with_help a_number
+      ]
+
+      expect(integration.api_field_names).to match_array %w[
+        foo foo_p foo_dt with_help a_number
+      ]
+    end
+
+    specify 'fields have expected attributes' do
+      expect(integration.fields).to include(
+        have_attributes(name: 'foo', type: 'text'),
+        have_attributes(name: 'bar', type: 'password'),
+        have_attributes(name: 'password', type: 'password'),
+        have_attributes(name: 'a_number', type: 'number'),
+        have_attributes(name: 'with_help', help: 'help')
+      )
+    end
+  end
 end

spec/models/integrations/field_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ::Integrations::Field do
+  subject(:field) { described_class.new(**attrs) }
+
+  let(:attrs) { { name: nil } }
+
+  describe '#name' do
+    before do
+      attrs[:name] = :foo
+    end
+
+    it 'is stringified' do
+      expect(field.name).to eq 'foo'
+      expect(field[:name]).to eq 'foo'
+    end
+
+    context 'when not set' do
+      before do
+        attrs.delete(:name)
+      end
+
+      it 'complains' do
+        expect { field }.to raise_error(ArgumentError)
+      end
+    end
+  end
+
+  described_class::ATTRIBUTES.each do |name|
+    describe "##{name}" do
+      it "responds to #{name}" do
+        expect(field).to be_respond_to(name)
+      end
+
+      context 'when not set' do
+        before do
+          attrs.delete(name)
+        end
+
+        let(:have_correct_default) do
+          case name
+          when :api_only
+            be false
+          when :type
+            eq 'text'
+          else
+            be_nil
+          end
+        end
+
+        it 'has the correct default' do
+          expect(field[name]).to have_correct_default
+          expect(field.send(name)).to have_correct_default
+        end
+      end
+
+      context 'when set to a static value' do
+        before do
+          attrs[name] = :known
+        end
+
+        it 'is known' do
+          expect(field[name]).to eq(:known)
+          expect(field.send(name)).to eq(:known)
+        end
+      end
+
+      context 'when set to a dynamic value' do
+        before do
+          attrs[name] = -> { Time.current }
+        end
+
+        it 'is computed' do
+          start = Time.current
+
+          travel_to(start + 1.minute) do
+            expect(field[name]).to be_after(start)
+            expect(field.send(name)).to be_after(start)
+          end
+        end
+      end
+    end
+  end
+
+  describe '#sensitive' do
+    context 'when empty' do
+      it { is_expected.not_to be_sensitive }
+    end
+
+    context 'when a password field' do
+      before do
+        attrs[:type] = 'password'
+      end
+
+      it { is_expected.to be_sensitive }
+    end
+
+    %w[token api_token api_key secret_key secret_sauce password passphrase].each do |name|
+      context "when named #{name}" do
+        before do
+          attrs[:name] = name
+        end
+
+        it { is_expected.to be_sensitive }
+      end
+    end
+
+    context "when named url" do
+      before do
+        attrs[:name] = :url
+      end
+
+      it { is_expected.not_to be_sensitive }
+    end
+  end
+end