From 554df8d4d6e17af4e9ee3b6be87138e65a0a93c0 Mon Sep 17 00:00:00 2001 From: Oghenerukevwe Kofi Date: Wed, 16 Jun 2021 11:19:57 +0100 Subject: [PATCH 1/6] Properly return boolean attributes in integrations API Changelog: fixed --- app/models/integration.rb | 22 +++++++++++++--------- lib/api/entities/project_service.rb | 16 ++++++++++++---- spec/requests/api/services_spec.rb | 22 ++++++++++++++++++++++ 3 files changed, 47 insertions(+), 13 deletions(-) diff --git a/app/models/integration.rb b/app/models/integration.rb index 2fbcdc7f1cb35b..0c7f288b4de497 100644 --- a/app/models/integration.rb +++ b/app/models/integration.rb @@ -156,9 +156,13 @@ def self.boolean_accessor(*args) args.each do |arg| class_eval <<~RUBY, __FILE__, __LINE__ + 1 + def #{arg} + ActiveRecord::Type::Boolean.new.cast(properties['#{arg}']) + end + def #{arg}? # '!!' is used because nil or empty string is converted to nil - !!ActiveRecord::Type::Boolean.new.cast(#{arg}) + !!#{arg} end RUBY end @@ -334,10 +338,10 @@ def self.create_from_active_default_integrations(scope, association, with_templa array = group_ids.to_sql.present? ? "array(#{group_ids.to_sql})" : 'ARRAY[]' from_union([ - with_templates ? active.where(template: true) : none, - active.where(instance: true), - active.where(group_id: group_ids, inherit_from_id: nil) - ]).order(Arel.sql("type ASC, array_position(#{array}::bigint[], services.group_id), instance DESC")).group_by(&:type).each do |type, records| + with_templates ? active.where(template: true) : none, + active.where(instance: true), + active.where(group_id: group_ids, inherit_from_id: nil) + ]).order(Arel.sql("type ASC, array_position(#{array}::bigint[], services.group_id), instance DESC")).group_by(&:type).each do |type, records| build_from_integration(records.first, association => scope.id).save end end @@ -348,9 +352,9 @@ def self.inherited_descendants_from_self_or_ancestors_from(integration) .or(where(type: integration.type, instance: true)).select(:id) from_union([ - where(type: integration.type, inherit_from_id: inherit_from_ids, group: integration.group.descendants), - where(type: integration.type, inherit_from_id: inherit_from_ids, project: Project.in_namespace(integration.group.self_and_descendants)) - ]) + where(type: integration.type, inherit_from_id: inherit_from_ids, group: integration.group.descendants), + where(type: integration.type, inherit_from_id: inherit_from_ids, project: Project.in_namespace(integration.group.self_and_descendants)) + ]) end def activated? @@ -428,7 +432,7 @@ def event_field(event) def api_field_names fields.map { |field| field[:name] } - .reject { |field_name| field_name =~ /(password|token|key|title|description)/ } + .reject { |field_name| field_name =~ /(password|token|key|title|description)/ } end def global_fields diff --git a/lib/api/entities/project_service.rb b/lib/api/entities/project_service.rb index 947cec1e3cd199..07abf23660de71 100644 --- a/lib/api/entities/project_service.rb +++ b/lib/api/entities/project_service.rb @@ -6,10 +6,18 @@ class ProjectService < Entities::ProjectServiceBasic # Expose serialized properties expose :properties do |service, options| # TODO: Simplify as part of https://gitlab.com/gitlab-org/gitlab/issues/29404 - if service.data_fields_present? - service.data_fields.as_json.slice(*service.api_field_names) - else - service.properties.slice(*service.api_field_names) + + attributes = + if service.data_fields_present? + service.data_fields.as_json.keys + else + service.properties.keys + end + + attributes &= service.api_field_names + + attributes.each_with_object({}) do |attribute, hash| + hash[attribute] = service.public_send(attribute) # rubocop:disable GitlabSecurity/PublicSend end end end diff --git a/spec/requests/api/services_spec.rb b/spec/requests/api/services_spec.rb index f7394fa0cb4d34..0c81fe2af0ff32 100644 --- a/spec/requests/api/services_spec.rb +++ b/spec/requests/api/services_spec.rb @@ -336,4 +336,26 @@ def deactive_service! expect(response).to have_gitlab_http_status(:ok) end end + + describe 'Pipelines Email service' do + let(:service_name) { 'pipelines-email' } + + context 'notify_only_broken_pipelines property was saved as a string' do + before do + project.create_pipelines_email_service( + active: false, + properties: { + "notify_only_broken_pipelines": "true", + "branches_to_be_notified": "default" + } + ) + end + + it 'returns boolean values for notify_only_broken_pipelines' do + get api("/projects/#{project.id}/services/#{service_name}", user) + + expect(json_response['properties']['notify_only_broken_pipelines']).to eq(true) + end + end + end end -- GitLab From 27267d8173d52b186b8d0fbe5df333f59a6db620 Mon Sep 17 00:00:00 2001 From: Allison Browne Date: Tue, 22 Jun 2021 08:06:54 +0000 Subject: [PATCH 2/6] use Gitlab utils for Integration boolean_attributes --- app/models/integration.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/integration.rb b/app/models/integration.rb index 0c7f288b4de497..c2913f73592e7a 100644 --- a/app/models/integration.rb +++ b/app/models/integration.rb @@ -157,7 +157,7 @@ def self.boolean_accessor(*args) args.each do |arg| class_eval <<~RUBY, __FILE__, __LINE__ + 1 def #{arg} - ActiveRecord::Type::Boolean.new.cast(properties['#{arg}']) + Gitlab::Utils.to_boolean(properties['#{arg}']) end def #{arg}? -- GitLab From b05da01ff32bec9138b482eb13db14fcac37f53e Mon Sep 17 00:00:00 2001 From: Oghenerukevwe Kofi Date: Wed, 16 Jun 2021 11:19:57 +0100 Subject: [PATCH 3/6] Properly return boolean attributes in integrations API Changelog: fixed Properly return boolean attributes in integrations API Properly return boolean attributes in integrations API remove boolean attribute initialization from pipelines_email --- app/models/integration.rb | 19 ++++++++++++++++--- lib/api/entities/project_service.rb | 16 ++++++++++++---- spec/requests/api/services_spec.rb | 22 ++++++++++++++++++++++ 3 files changed, 50 insertions(+), 7 deletions(-) diff --git a/app/models/integration.rb b/app/models/integration.rb index 2fbcdc7f1cb35b..0cbe092d140988 100644 --- a/app/models/integration.rb +++ b/app/models/integration.rb @@ -14,14 +14,14 @@ class Integration < ApplicationRecord self.table_name = 'services' INTEGRATION_NAMES = %w[ - asana assembla bamboo bugzilla buildkite campfire confluence custom_issue_tracker discord + asana assembla bamboo bugzilla buildkite campfire confluence custom_issue_tracker datadog discord drone_ci emails_on_push ewm external_wiki flowdock hangouts_chat irker jira mattermost mattermost_slash_commands microsoft_teams packagist pipelines_email pivotaltracker prometheus pushover redmine slack slack_slash_commands teamcity unify_circuit webex_teams youtrack ].freeze PROJECT_SPECIFIC_INTEGRATION_NAMES = %w[ - datadog jenkins + jenkins ].freeze # Fake integrations to help with local development. @@ -48,6 +48,15 @@ class Integration < ApplicationRecord flowdock hangouts_chat irker + jenkins jira + packagist pipelines_email pivotaltracker prometheus pushover + mattermost mattermost_slash_commands microsoft_teams mock_ci mock_monitoring + redmine + slack slack_slash_commands + teamcity + unify_circuit + webex_teams + youtrack ].to_set.freeze def self.renamed?(name) @@ -156,9 +165,13 @@ def self.boolean_accessor(*args) args.each do |arg| class_eval <<~RUBY, __FILE__, __LINE__ + 1 + def #{arg} + Gitlab::Utils.to_boolean(properties['#{arg}']) + end + def #{arg}? # '!!' is used because nil or empty string is converted to nil - !!ActiveRecord::Type::Boolean.new.cast(#{arg}) + !!#{arg} end RUBY end diff --git a/lib/api/entities/project_service.rb b/lib/api/entities/project_service.rb index 947cec1e3cd199..07abf23660de71 100644 --- a/lib/api/entities/project_service.rb +++ b/lib/api/entities/project_service.rb @@ -6,10 +6,18 @@ class ProjectService < Entities::ProjectServiceBasic # Expose serialized properties expose :properties do |service, options| # TODO: Simplify as part of https://gitlab.com/gitlab-org/gitlab/issues/29404 - if service.data_fields_present? - service.data_fields.as_json.slice(*service.api_field_names) - else - service.properties.slice(*service.api_field_names) + + attributes = + if service.data_fields_present? + service.data_fields.as_json.keys + else + service.properties.keys + end + + attributes &= service.api_field_names + + attributes.each_with_object({}) do |attribute, hash| + hash[attribute] = service.public_send(attribute) # rubocop:disable GitlabSecurity/PublicSend end end end diff --git a/spec/requests/api/services_spec.rb b/spec/requests/api/services_spec.rb index f7394fa0cb4d34..0c81fe2af0ff32 100644 --- a/spec/requests/api/services_spec.rb +++ b/spec/requests/api/services_spec.rb @@ -336,4 +336,26 @@ def deactive_service! expect(response).to have_gitlab_http_status(:ok) end end + + describe 'Pipelines Email service' do + let(:service_name) { 'pipelines-email' } + + context 'notify_only_broken_pipelines property was saved as a string' do + before do + project.create_pipelines_email_service( + active: false, + properties: { + "notify_only_broken_pipelines": "true", + "branches_to_be_notified": "default" + } + ) + end + + it 'returns boolean values for notify_only_broken_pipelines' do + get api("/projects/#{project.id}/services/#{service_name}", user) + + expect(json_response['properties']['notify_only_broken_pipelines']).to eq(true) + end + end + end end -- GitLab From 957b808420ffddef16c58f4270898fbc1e60e2ec Mon Sep 17 00:00:00 2001 From: Oghenerukevwe Kofi Date: Tue, 22 Jun 2021 14:54:02 +0100 Subject: [PATCH 4/6] fix whitespace issue in integration.rb --- app/models/integration.rb | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/app/models/integration.rb b/app/models/integration.rb index a8326a23ff5e3f..0cbe092d140988 100644 --- a/app/models/integration.rb +++ b/app/models/integration.rb @@ -347,10 +347,10 @@ def self.create_from_active_default_integrations(scope, association, with_templa array = group_ids.to_sql.present? ? "array(#{group_ids.to_sql})" : 'ARRAY[]' from_union([ - with_templates ? active.where(template: true) : none, - active.where(instance: true), - active.where(group_id: group_ids, inherit_from_id: nil) - ]).order(Arel.sql("type ASC, array_position(#{array}::bigint[], services.group_id), instance DESC")).group_by(&:type).each do |type, records| + with_templates ? active.where(template: true) : none, + active.where(instance: true), + active.where(group_id: group_ids, inherit_from_id: nil) + ]).order(Arel.sql("type ASC, array_position(#{array}::bigint[], services.group_id), instance DESC")).group_by(&:type).each do |type, records| build_from_integration(records.first, association => scope.id).save end end @@ -361,9 +361,9 @@ def self.inherited_descendants_from_self_or_ancestors_from(integration) .or(where(type: integration.type, instance: true)).select(:id) from_union([ - where(type: integration.type, inherit_from_id: inherit_from_ids, group: integration.group.descendants), - where(type: integration.type, inherit_from_id: inherit_from_ids, project: Project.in_namespace(integration.group.self_and_descendants)) - ]) + where(type: integration.type, inherit_from_id: inherit_from_ids, group: integration.group.descendants), + where(type: integration.type, inherit_from_id: inherit_from_ids, project: Project.in_namespace(integration.group.self_and_descendants)) + ]) end def activated? @@ -441,7 +441,7 @@ def event_field(event) def api_field_names fields.map { |field| field[:name] } - .reject { |field_name| field_name =~ /(password|token|key|title|description)/ } + .reject { |field_name| field_name =~ /(password|token|key|title|description)/ } end def global_fields -- GitLab From be789ba6e544189da4f46bacccf0b4e4620af0c9 Mon Sep 17 00:00:00 2001 From: Oghenerukevwe Kofi Date: Tue, 22 Jun 2021 16:10:27 +0100 Subject: [PATCH 5/6] fix pipelines_email integration in services spec --- spec/requests/api/services_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/api/services_spec.rb b/spec/requests/api/services_spec.rb index 7a4af41b6e92e6..1732e54ee10815 100644 --- a/spec/requests/api/services_spec.rb +++ b/spec/requests/api/services_spec.rb @@ -344,7 +344,7 @@ def deactive_service! context 'notify_only_broken_pipelines property was saved as a string' do before do - project.create_pipelines_email_service( + project.create_pipelines_email_integration( active: false, properties: { "notify_only_broken_pipelines": "true", -- GitLab From 221d9b9521fc4fdfac62da53ff2693c728ad4e0c Mon Sep 17 00:00:00 2001 From: Oghenerukevwe Kofi Date: Tue, 22 Jun 2021 16:10:41 +0100 Subject: [PATCH 6/6] fix pipelines_email integration in services spec --- spec/requests/api/services_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/api/services_spec.rb b/spec/requests/api/services_spec.rb index 1732e54ee10815..f4c0384a61b427 100644 --- a/spec/requests/api/services_spec.rb +++ b/spec/requests/api/services_spec.rb @@ -339,7 +339,7 @@ def deactive_service! end end - describe 'Pipelines Email service' do + describe 'Pipelines Email Integration' do let(:service_name) { 'pipelines-email' } context 'notify_only_broken_pipelines property was saved as a string' do -- GitLab