From c4b51ab0168e5e0fd8a506b52df05cfa1dc7c70b Mon Sep 17 00:00:00 2001
From: Markus Koller <mkoller@gitlab.com>
Date: Mon, 13 Jun 2022 09:08:52 +0000
Subject: [PATCH] Update specs for integrations API to check all modifiable
fields
Previously only the event booleans were verified, but not any other
attributes that were sent in the PUT request.
This is a step towards fixing inconsistencies in the Integrations API,
some attributes are currently not supported and skipped in the specs.
This change also exposed a bug in the getters defined by `data_field`,
where we'd incorrectly fallback to `properties` for falsy values. This
should only be done for explicit `nil` values.
Changelog: changed
---
.../concerns/integrations/has_data_fields.rb | 3 +-
.../integrations/has_data_fields_spec.rb | 88 +++++++++----------
spec/requests/api/integrations_spec.rb | 52 +++++++----
.../integrations_shared_context.rb | 36 +++++++-
4 files changed, 111 insertions(+), 68 deletions(-)
diff --git a/app/models/concerns/integrations/has_data_fields.rb b/app/models/concerns/integrations/has_data_fields.rb
index 25a1d8551197e..635147a2f3ce9 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 b28fef571c65f..374c5c33b5027 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 96cc101e73a98..cd9a074658185 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 a644e5d5b9c3b..3d3b8c2207d43 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")
--
GitLab