From 83126908715b1182415096a992eef00673a3d62a Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Thu, 24 Feb 2022 16:22:58 +0000 Subject: [PATCH] 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. --- .../integrations/has_issue_tracker_fields.rb | 30 ++++ app/models/integration.rb | 38 ++++- app/models/integrations/base_issue_tracker.rb | 12 -- app/models/integrations/bugzilla.rb | 2 + .../integrations/custom_issue_tracker.rb | 2 + app/models/integrations/ewm.rb | 2 + app/models/integrations/field.rb | 42 +++++ app/models/integrations/jira.rb | 89 +++++------ app/models/integrations/redmine.rb | 2 + app/models/integrations/youtrack.rb | 2 + spec/models/integration_spec.rb | 148 +++++++++++++++--- spec/models/integrations/field_spec.rb | 118 ++++++++++++++ 12 files changed, 396 insertions(+), 91 deletions(-) create mode 100644 app/models/concerns/integrations/has_issue_tracker_fields.rb create mode 100644 app/models/integrations/field.rb create mode 100644 spec/models/integrations/field_spec.rb diff --git a/app/models/concerns/integrations/has_issue_tracker_fields.rb b/app/models/concerns/integrations/has_issue_tracker_fields.rb new file mode 100644 index 00000000000000..b1def38d0193ac --- /dev/null +++ b/app/models/concerns/integrations/has_issue_tracker_fields.rb @@ -0,0 +1,30 @@ +# 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: ':id'.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 diff --git a/app/models/integration.rb b/app/models/integration.rb index fb44d90139ae99..3cf71f46420f6e 100644 --- a/app/models/integration.rb +++ b/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 @@ def to_param self.class.to_param end - def fields - # implement inside child - [] - end - def sections [] end diff --git a/app/models/integrations/base_issue_tracker.rb b/app/models/integrations/base_issue_tracker.rb index 71329f34dc1d58..458d0199e7a40a 100644 --- a/app/models/integrations/base_issue_tracker.rb +++ b/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 @@ def issue_path(iid) 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: ':id'.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 diff --git a/app/models/integrations/bugzilla.rb b/app/models/integrations/bugzilla.rb index 1b711264e10846..74e282f6848aaa 100644 --- a/app/models/integrations/bugzilla.rb +++ b/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 diff --git a/app/models/integrations/custom_issue_tracker.rb b/app/models/integrations/custom_issue_tracker.rb index 0299a50605f32c..3770e813eaaaeb 100644 --- a/app/models/integrations/custom_issue_tracker.rb +++ b/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 diff --git a/app/models/integrations/ewm.rb b/app/models/integrations/ewm.rb index c6ea989a03f351..1b86ef73c85199 100644 --- a/app/models/integrations/ewm.rb +++ b/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) diff --git a/app/models/integrations/field.rb b/app/models/integrations/field.rb new file mode 100644 index 00000000000000..49ab97677db4e0 --- /dev/null +++ b/app/models/integrations/field.rb @@ -0,0 +1,42 @@ +# 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 diff --git a/app/models/integrations/jira.rb b/app/models/integrations/jira.rb index 8fc0a3461f763e..7308c731bb09bb 100644 --- a/app/models/integrations/jira.rb +++ b/app/models/integrations/jira.rb @@ -28,11 +28,6 @@ class Jira < BaseIssueTracker # 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 @@ class Jira < BaseIssueTracker 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 @@ def self.to_param '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 @@ def web_url(path = nil, **params) 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 diff --git a/app/models/integrations/redmine.rb b/app/models/integrations/redmine.rb index 3812033c3880ea..bc2a64b084891c 100644 --- a/app/models/integrations/redmine.rb +++ b/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 diff --git a/app/models/integrations/youtrack.rb b/app/models/integrations/youtrack.rb index 254f0c6c69a143..ab6e1da27f8404 100644 --- a/app/models/integrations/youtrack.rb +++ b/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 diff --git a/spec/models/integration_spec.rb b/spec/models/integration_spec.rb index 3d85bd245d901f..48d8ba975b68ae 100644 --- a/spec/models/integration_spec.rb +++ b/spec/models/integration_spec.rb @@ -719,34 +719,63 @@ 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 @@ def fields 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 diff --git a/spec/models/integrations/field_spec.rb b/spec/models/integrations/field_spec.rb new file mode 100644 index 00000000000000..0d660c4a3ab32e --- /dev/null +++ b/spec/models/integrations/field_spec.rb @@ -0,0 +1,118 @@ +# 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 -- GitLab