diff --git a/app/models/concerns/integrations/has_data_fields.rb b/app/models/concerns/integrations/has_data_fields.rb
index 25a1d8551197e5af6c70d0f046a2051d3918677a..635147a2f3ce9f9cc1eb5f791d642e7592d47f46 100644
--- a/app/models/concerns/integrations/has_data_fields.rb
+++ b/app/models/concerns/integrations/has_data_fields.rb
@@ -12,7 +12,8 @@ def data_field(*args)
self.class_eval <<-RUBY, __FILE__, __LINE__ + 1
unless method_defined?(arg)
def #{arg}
- data_fields.send('#{arg}') || (properties && properties['#{arg}'])
+ value = data_fields.send('#{arg}')
+ value.nil? ? properties&.dig('#{arg}') : value
end
end
diff --git a/spec/models/concerns/integrations/has_data_fields_spec.rb b/spec/models/concerns/integrations/has_data_fields_spec.rb
index b28fef571c65fccf34c01e9b115c2be87dbcc544..374c5c33b5027ffb25bc53a3a3fe7e9acbc2793e 100644
--- a/spec/models/concerns/integrations/has_data_fields_spec.rb
+++ b/spec/models/concerns/integrations/has_data_fields_spec.rb
@@ -6,84 +6,84 @@
let(:url) { 'http://url.com' }
let(:username) { 'username_one' }
let(:properties) do
- { url: url, username: username }
+ { url: url, username: username, jira_issue_transition_automatic: false }
end
shared_examples 'data fields' do
describe '#arg' do
- it 'returns an argument correctly' do
- expect(service.url).to eq(url)
+ it 'returns the expected values' do
+ expect(integration).to have_attributes(properties)
end
end
describe '{arg}_changed?' do
it 'returns false when the property has not been assigned a new value' do
- service.username = 'new_username'
- service.validate
- expect(service.url_changed?).to be_falsy
+ integration.username = 'new_username'
+ integration.validate
+ expect(integration.url_changed?).to be_falsy
end
it 'returns true when the property has been assigned a different value' do
- service.url = "http://example.com"
- service.validate
- expect(service.url_changed?).to be_truthy
+ integration.url = "http://example.com"
+ integration.validate
+ expect(integration.url_changed?).to be_truthy
end
it 'returns true when the property has been assigned a different value twice' do
- service.url = "http://example.com"
- service.url = "http://example.com"
- service.validate
- expect(service.url_changed?).to be_truthy
+ integration.url = "http://example.com"
+ integration.url = "http://example.com"
+ integration.validate
+ expect(integration.url_changed?).to be_truthy
end
it 'returns false when the property has been re-assigned the same value' do
- service.url = 'http://url.com'
- service.validate
- expect(service.url_changed?).to be_falsy
+ integration.url = 'http://url.com'
+ integration.validate
+ expect(integration.url_changed?).to be_falsy
end
end
describe '{arg}_touched?' do
it 'returns false when the property has not been assigned a new value' do
- service.username = 'new_username'
- service.validate
- expect(service.url_changed?).to be_falsy
+ integration.username = 'new_username'
+ integration.validate
+ expect(integration.url_changed?).to be_falsy
end
it 'returns true when the property has been assigned a different value' do
- service.url = "http://example.com"
- service.validate
- expect(service.url_changed?).to be_truthy
+ integration.url = "http://example.com"
+ integration.validate
+ expect(integration.url_changed?).to be_truthy
end
it 'returns true when the property has been assigned a different value twice' do
- service.url = "http://example.com"
- service.url = "http://example.com"
- service.validate
- expect(service.url_changed?).to be_truthy
+ integration.url = "http://example.com"
+ integration.url = "http://example.com"
+ integration.validate
+ expect(integration.url_changed?).to be_truthy
end
it 'returns true when the property has been re-assigned the same value' do
- service.url = 'http://url.com'
- expect(service.url_touched?).to be_truthy
+ integration.url = 'http://url.com'
+ expect(integration.url_touched?).to be_truthy
end
it 'returns false when the property has been re-assigned the same value' do
- service.url = 'http://url.com'
- service.validate
- expect(service.url_changed?).to be_falsy
+ integration.url = 'http://url.com'
+ integration.validate
+ expect(integration.url_changed?).to be_falsy
end
end
describe 'data_fields_present?' do
- it 'returns true from the issue tracker service' do
- expect(service.data_fields_present?).to be true
+ it 'returns true from the issue tracker integration' do
+ expect(integration.data_fields_present?).to be true
end
end
end
context 'when data are stored in data_fields' do
- let(:service) do
+ let(:integration) do
create(:jira_integration, url: url, username: username)
end
@@ -91,21 +91,21 @@
describe '{arg}_was?' do
it 'returns nil' do
- service.url = 'http://example.com'
- service.validate
- expect(service.url_was).to be_nil
+ integration.url = 'http://example.com'
+ integration.validate
+ expect(integration.url_was).to be_nil
end
end
end
- context 'when service and data_fields are not persisted' do
- let(:service) do
+ context 'when integration and data_fields are not persisted' do
+ let(:integration) do
Integrations::Jira.new
end
describe 'data_fields_present?' do
it 'returns true' do
- expect(service.data_fields_present?).to be true
+ expect(integration.data_fields_present?).to be true
end
end
end
@@ -113,9 +113,7 @@
context 'when data are stored in properties' do
let(:integration) { create(:jira_integration, :without_properties_callback, properties: properties) }
- it_behaves_like 'data fields' do
- let(:service) { integration }
- end
+ it_behaves_like 'data fields'
describe '{arg}_was?' do
it 'returns nil when the property has not been assigned a new value' do
@@ -148,9 +146,7 @@
end
end
- it_behaves_like 'data fields' do
- let(:service) { integration }
- end
+ it_behaves_like 'data fields'
describe '{arg}_was?' do
it 'returns nil' do
diff --git a/spec/requests/api/integrations_spec.rb b/spec/requests/api/integrations_spec.rb
index 96cc101e73a982a1687b0fd5b27856f509b46ecf..cd9a074658185eaa58ef67c632226d9c21600bd4 100644
--- a/spec/requests/api/integrations_spec.rb
+++ b/spec/requests/api/integrations_spec.rb
@@ -55,33 +55,49 @@ def self.integration_names
describe "PUT /projects/:id/#{endpoint}/#{integration.dasherize}" do
include_context integration
+ # NOTE: Some attributes are not supported for PUT requests, even though in most cases they should be.
+ # For some of them the problem is somewhere else, i.e. most chat integrations don't support the `*_channel`
+ # fields but they're incorrectly included in `#fields`.
+ #
+ # We can fix these manually, or with a generic approach like https://gitlab.com/gitlab-org/gitlab/-/issues/348208
+ let(:missing_channel_attributes) { %i[push_channel issue_channel confidential_issue_channel merge_request_channel note_channel confidential_note_channel tag_push_channel pipeline_channel wiki_page_channel] }
+ let(:missing_attributes) do
+ {
+ datadog: %i[archive_trace_events],
+ discord: missing_channel_attributes + %i[branches_to_be_notified notify_only_broken_pipelines],
+ hangouts_chat: missing_channel_attributes + %i[notify_only_broken_pipelines],
+ jira: %i[issues_enabled project_key vulnerabilities_enabled vulnerabilities_issuetype],
+ mattermost: %i[deployment_channel labels_to_be_notified],
+ microsoft_teams: missing_channel_attributes,
+ mock_ci: %i[enable_ssl_verification],
+ prometheus: %i[manual_configuration],
+ slack: %i[alert_events alert_channel deployment_channel labels_to_be_notified],
+ unify_circuit: missing_channel_attributes + %i[branches_to_be_notified notify_only_broken_pipelines],
+ webex_teams: missing_channel_attributes + %i[branches_to_be_notified notify_only_broken_pipelines]
+ }
+ end
+
it "updates #{integration} settings and returns the correct fields" do
- put api("/projects/#{project.id}/#{endpoint}/#{dashed_integration}", user), params: integration_attrs
+ supported_attrs = integration_attrs.without(missing_attributes.fetch(integration.to_sym, []))
+
+ put api("/projects/#{project.id}/#{endpoint}/#{dashed_integration}", user), params: supported_attrs
expect(response).to have_gitlab_http_status(:ok)
+ expect(json_response['slug']).to eq(dashed_integration)
current_integration = project.integrations.first
- events = current_integration.event_names.empty? ? ["foo"].freeze : current_integration.event_names
- query_strings = []
- events.map(&:to_sym).each do |event|
- event_value = !current_integration[event]
- query_strings << "#{event}=#{event_value}"
- integration_attrs[event] = event_value if integration_attrs[event].present?
+ expect(current_integration).to have_attributes(supported_attrs)
+ assert_correct_response_fields(json_response['properties'].keys, current_integration)
+
+ # Flip all booleans and verify that we can set these too
+ flipped_attrs = supported_attrs.transform_values do |value|
+ [true, false].include?(value) ? !value : value
end
- query_strings = query_strings.join('&')
- put api("/projects/#{project.id}/#{endpoint}/#{dashed_integration}?#{query_strings}", user), params: integration_attrs
+ put api("/projects/#{project.id}/#{endpoint}/#{dashed_integration}", user), params: flipped_attrs
expect(response).to have_gitlab_http_status(:ok)
- expect(json_response['slug']).to eq(dashed_integration)
- events.each do |event|
- next if event == "foo"
-
- expect(project.integrations.first[event]).not_to eq(current_integration[event]),
- "expected #{!current_integration[event]} for event #{event} for #{endpoint} #{current_integration.title}, got #{current_integration[event]}"
- end
-
- assert_correct_response_fields(json_response['properties'].keys, current_integration)
+ expect(project.integrations.first).to have_attributes(flipped_attrs)
end
it "returns if required fields missing" do
diff --git a/spec/support/shared_contexts/features/integrations/integrations_shared_context.rb b/spec/support/shared_contexts/features/integrations/integrations_shared_context.rb
index a644e5d5b9c3b53d3f892cfdd0f6d268f3b421a1..3d3b8c2207d438e24951550670e2818f3c63b7f9 100644
--- a/spec/support/shared_contexts/features/integrations/integrations_shared_context.rb
+++ b/spec/support/shared_contexts/features/integrations/integrations_shared_context.rb
@@ -8,8 +8,36 @@
let(:integration_method) { Project.integration_association_name(integration) }
let(:integration_klass) { Integration.integration_name_to_model(integration) }
let(:integration_instance) { integration_klass.new }
- let(:integration_fields) { integration_instance.fields }
- let(:integration_attrs_list) { integration_fields.inject([]) {|arr, hash| arr << hash[:name].to_sym } }
+
+ # Build a list of all attributes that an integration supports.
+ let(:integration_attrs_list) do
+ integration_fields + integration_events + custom_attributes.fetch(integration.to_sym, [])
+ end
+
+ # Attributes defined as fields.
+ let(:integration_fields) do
+ integration_instance.fields.map { _1[:name].to_sym }
+ end
+
+ # Attributes for configurable event triggers.
+ let(:integration_events) do
+ integration_instance.configurable_events.map { IntegrationsHelper.integration_event_field_name(_1).to_sym }
+ end
+
+ # Other special cases, this list might be incomplete.
+ #
+ # Some of these won't be needed anymore after we've converted them to use the field DSL
+ # in https://gitlab.com/gitlab-org/gitlab/-/issues/354899.
+ #
+ # Others like `comment_on_event_disabled` are actual columns on `integrations`, maybe we should migrate
+ # these to fields as well.
+ let(:custom_attributes) do
+ {
+ jira: %i[comment_on_event_enabled jira_issue_transition_automatic jira_issue_transition_id project_key
+ issues_enabled vulnerabilities_enabled vulnerabilities_issuetype]
+ }
+ end
+
let(:integration_attrs) do
integration_attrs_list.inject({}) do |hash, k|
if k =~ /^(token*|.*_token|.*_key)/
@@ -32,9 +60,11 @@
hash.merge!(k => 1234)
elsif integration == 'jira' && k == :jira_issue_transition_id
hash.merge!(k => '1,2,3')
+ elsif integration == 'jira' && k == :jira_issue_transition_automatic
+ hash.merge!(k => true)
elsif integration == 'emails_on_push' && k == :recipients
hash.merge!(k => 'foo@bar.com')
- elsif integration == 'slack' || integration == 'mattermost' && k == :labels_to_be_notified_behavior
+ elsif (integration == 'slack' || integration == 'mattermost') && k == :labels_to_be_notified_behavior
hash.merge!(k => "match_any")
else
hash.merge!(k => "someword")