Fix several issues on Datadog integration service form

* Adding blank query parameters `env` and `service` to the URL
* Accepting blank string on the `:datadog_site` field
* Wrong validation of `:datadog_site` and `:api_url` together when both are blank strings
* Default value for `:datadog_site` not working as intended.
* Test button hangs when testing before saving, with invalid data.

app/models/project_services/datadog_service.rb

@@ -12,14 +12,22 @@ class DatadogService < Service
 
   prop_accessor :datadog_site, :api_url, :api_key, :datadog_service, :datadog_env
 
-  with_options presence: true, if: :activated? do
-    validates :api_key, format: { with: /\A\w+\z/ }
-    validates :datadog_site, format: { with: /\A[\w\.]+\z/ }, unless: :api_url
-    validates :api_url, public_url: true, unless: :datadog_site
+  with_options if: :activated? do
+    validates :api_key, presence: true, format: { with: /\A\w+\z/ }
+    validates :datadog_site, format: { with: /\A[\w\.]+\z/, allow_blank: true }
+    validates :api_url, public_url: { allow_blank: true }
+    validates :datadog_site, presence: true, unless: -> (obj) { obj.api_url.present? }
+    validates :api_url, presence: true, unless: -> (obj) { obj.datadog_site.present? }
   end
 
   after_save :compose_service_hook, if: :activated?
 
+  def initialize_properties
+    super
+
+    self.datadog_site ||= DEFAULT_SITE
+  end
+
   def self.supported_events
     SUPPORTED_EVENTS
   end
@@ -54,27 +62,37 @@ class DatadogService < Service
   def fields
     [
       {
-        type: 'text', name: 'datadog_site',
-        placeholder: DEFAULT_SITE, default: DEFAULT_SITE,
+        type: 'text',
+        name: 'datadog_site',
+        placeholder: DEFAULT_SITE,
         help: 'Choose the Datadog site to send data to. Set to "datadoghq.eu" to send data to the EU site',
         required: false
       },
       {
-        type: 'text', name: 'api_url', title: 'Custom URL',
+        type: 'text',
+        name: 'api_url',
+        title: 'API URL',
         help: '(Advanced) Define the full URL for your Datadog site directly',
         required: false
       },
       {
-        type: 'password', name: 'api_key', title: 'API key',
+        type: 'password',
+        name: 'api_key',
+        title: 'API key',
         help: "<a href=\"#{api_keys_url}\" target=\"_blank\">API key</a> used for authentication with Datadog",
         required: true
       },
       {
-        type: 'text', name: 'datadog_service', title: 'Service', placeholder: 'gitlab-ci',
+        type: 'text',
+        name: 'datadog_service',
+        title: 'Service',
+        placeholder: 'gitlab-ci',
         help: 'Name of this GitLab instance that all data will be tagged with'
       },
       {
-        type: 'text', name: 'datadog_env', title: 'Env',
+        type: 'text',
+        name: 'datadog_env',
+        title: 'Env',
         help: 'The environment tag that traces will be tagged with'
       }
     ]
@@ -90,7 +108,7 @@ class DatadogService < Service
     url = api_url.presence || sprintf(URL_TEMPLATE, datadog_site: datadog_site)
     url = URI.parse(url)
     url.path = File.join(url.path || '/', api_key)
-    query = { service: datadog_service, env: datadog_env }.compact
+    query = { service: datadog_service.presence, env: datadog_env.presence }.compact
     url.query = query.to_query unless query.empty?
     url.to_s
   end

spec/models/project_services/datadog_service_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe DatadogService, :model do
   let(:active) { true }
   let(:dd_site) { 'datadoghq.com' }
   let(:default_url) { 'https://webhooks-http-intake.logs.datadoghq.com/v1/input/' }
-  let(:api_url) { nil }
+  let(:api_url) { '' }
   let(:api_key) { SecureRandom.hex(32) }
   let(:dd_env) { 'ci' }
   let(:dd_service) { 'awesome-gitlab' }
@@ -22,13 +22,11 @@ RSpec.describe DatadogService, :model do
     described_class.new(
       active: active,
       project: project,
-      properties: {
-        datadog_site: dd_site,
-        api_url: api_url,
-        api_key: api_key,
-        datadog_env: dd_env,
-        datadog_service: dd_service
-      }
+      datadog_site: dd_site,
+      api_url: api_url,
+      api_key: api_key,
+      datadog_env: dd_env,
+      datadog_service: dd_service
     )
   end
 
@@ -58,7 +56,7 @@ RSpec.describe DatadogService, :model do
 
       context 'when selecting site' do
         let(:dd_site) { 'datadoghq.com' }
-        let(:api_url) { nil }
+        let(:api_url) { '' }
 
         it { is_expected.to validate_presence_of(:datadog_site) }
         it { is_expected.not_to validate_presence_of(:api_url) }
@@ -66,7 +64,7 @@ RSpec.describe DatadogService, :model do
       end
 
       context 'with custom api_url' do
-        let(:dd_site) { nil }
+        let(:dd_site) { '' }
         let(:api_url) { 'https://webhooks-http-intake.logs.datad0g.com/v1/input/' }
 
         it { is_expected.not_to validate_presence_of(:datadog_site) }
@@ -76,13 +74,21 @@ RSpec.describe DatadogService, :model do
       end
 
       context 'when missing site and api_url' do
-        let(:dd_site) { nil }
-        let(:api_url) { nil }
+        let(:dd_site) { '' }
+        let(:api_url) { '' }
 
         it { is_expected.not_to be_valid }
         it { is_expected.to validate_presence_of(:datadog_site) }
         it { is_expected.to validate_presence_of(:api_url) }
       end
+
+      context 'when providing both site and api_url' do
+        let(:dd_site) { 'datadoghq.com' }
+        let(:api_url) { default_url }
+
+        it { is_expected.not_to allow_value('datadog hq.com').for(:datadog_site) }
+        it { is_expected.not_to allow_value('example.com').for(:api_url) }
+      end
     end
 
     context 'when service is not active' do
@@ -113,8 +119,8 @@ RSpec.describe DatadogService, :model do
     end
 
     context 'without optional params' do
-      let(:dd_service) { nil }
-      let(:dd_env) { nil }
+      let(:dd_service) { '' }
+      let(:dd_env) { '' }
 
       it { is_expected.to eq(default_url + api_key) }
     end
@@ -126,7 +132,7 @@ RSpec.describe DatadogService, :model do
     it { is_expected.to eq("https://app.#{dd_site}/account/settings#api") }
 
     context 'with unset datadog_site' do
-      let(:dd_site) { nil }
+      let(:dd_site) { '' }
 
       it { is_expected.to eq("https://docs.datadoghq.com/account_management/api-app-keys/") }
     end