Merge branch 'master' into '24196-protected-variables'

# Conflicts:
#   db/schema.rb

12 files changed, 277 insertions, 139 deletions

diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 56b24ce62f3..c8023dc13b1 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -965,7 +965,7 @@ describe Ci::Pipeline, models: true do
     end
 
     before do
-      ProjectWebHookWorker.drain
+      WebHookWorker.drain
     end
 
     context 'with pipeline hooks enabled' do
diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb
index ab4c51a87b0..96f075d4f7d 100644
--- a/spec/models/diff_note_spec.rb
+++ b/spec/models/diff_note_spec.rb
@@ -145,7 +145,7 @@ describe DiffNote, models: true do
 
       context "when the merge request's diff refs don't match that of the diff note" do
         before do
-          allow(subject.noteable).to receive(:diff_sha_refs).and_return(commit.diff_refs)
+          allow(subject.noteable).to receive(:diff_refs).and_return(commit.diff_refs)
         end
 
         it "returns false" do
@@ -194,7 +194,7 @@ describe DiffNote, models: true do
 
         context "when the note is outdated" do
           before do
-            allow(merge_request).to receive(:diff_sha_refs).and_return(commit.diff_refs)
+            allow(merge_request).to receive(:diff_refs).and_return(commit.diff_refs)
           end
 
           it "uses the DiffPositionUpdateService" do
diff --git a/spec/models/hooks/service_hook_spec.rb b/spec/models/hooks/service_hook_spec.rb
index 1a83c836652..57454d2a773 100644
--- a/spec/models/hooks/service_hook_spec.rb
+++ b/spec/models/hooks/service_hook_spec.rb
@@ -1,36 +1,19 @@
-require "spec_helper"
+require 'spec_helper'
 
 describe ServiceHook, models: true do
-  describe "Associations" do
+  describe 'associations' do
     it { is_expected.to belong_to :service }
   end
 
-  describe "execute" do
-    before(:each) do
-      @service_hook = create(:service_hook)
-      @data = { project_id: 1, data: {} }
+  describe 'execute' do
+    let(:hook) { build(:service_hook) }
+    let(:data) { { key: 'value' } }
 
-      WebMock.stub_request(:post, @service_hook.url)
-    end
-
-    it "POSTs to the webhook URL" do
-      @service_hook.execute(@data)
-      expect(WebMock).to have_requested(:post, @service_hook.url).with(
-        headers: { 'Content-Type' => 'application/json', 'X-Gitlab-Event' => 'Service Hook' }
-      ).once
-    end
-
-    it "POSTs the data as JSON" do
-      @service_hook.execute(@data)
-      expect(WebMock).to have_requested(:post, @service_hook.url).with(
-        headers: { 'Content-Type' => 'application/json', 'X-Gitlab-Event' => 'Service Hook' }
-      ).once
-    end
-
-    it "catches exceptions" do
-      expect(WebHook).to receive(:post).and_raise("Some HTTP Post error")
+    it '#execute' do
+      expect(WebHookService).to receive(:new).with(hook, data, 'service_hook').and_call_original
+      expect_any_instance_of(WebHookService).to receive(:execute)
 
-      expect { @service_hook.execute(@data) }.to raise_error(RuntimeError)
+      hook.execute(data)
     end
   end
 end
diff --git a/spec/models/hooks/system_hook_spec.rb b/spec/models/hooks/system_hook_spec.rb
index 4340170888d..0d2b622132e 100644
--- a/spec/models/hooks/system_hook_spec.rb
+++ b/spec/models/hooks/system_hook_spec.rb
@@ -126,4 +126,26 @@ describe SystemHook, models: true do
       expect(SystemHook.repository_update_hooks).to eq([hook])
     end
   end
+
+  describe 'execute WebHookService' do
+    let(:hook) { build(:system_hook) }
+    let(:data) { { key: 'value' } }
+    let(:hook_name) { 'system_hook' }
+
+    before do
+      expect(WebHookService).to receive(:new).with(hook, data, hook_name).and_call_original
+    end
+
+    it '#execute' do
+      expect_any_instance_of(WebHookService).to receive(:execute)
+
+      hook.execute(data, hook_name)
+    end
+
+    it '#async_execute' do
+      expect_any_instance_of(WebHookService).to receive(:async_execute)
+
+      hook.async_execute(data, hook_name)
+    end
+  end
 end
diff --git a/spec/models/hooks/web_hook_log_spec.rb b/spec/models/hooks/web_hook_log_spec.rb
new file mode 100644
index 00000000000..c649cf3b589
--- /dev/null
+++ b/spec/models/hooks/web_hook_log_spec.rb
@@ -0,0 +1,30 @@
+require 'rails_helper'
+
+describe WebHookLog, models: true do
+  it { is_expected.to belong_to(:web_hook) }
+
+  it { is_expected.to serialize(:request_headers).as(Hash) }
+  it { is_expected.to serialize(:request_data).as(Hash) }
+  it { is_expected.to serialize(:response_headers).as(Hash) }
+
+  it { is_expected.to validate_presence_of(:web_hook) }
+
+  describe '#success?' do
+    let(:web_hook_log) { build(:web_hook_log, response_status: status) }
+
+    describe '2xx' do
+      let(:status) { '200' }
+      it { expect(web_hook_log.success?).to be_truthy }
+    end
+
+    describe 'not 2xx' do
+      let(:status) { '500' }
+      it { expect(web_hook_log.success?).to be_falsey }
+    end
+
+    describe 'internal erorr' do
+      let(:status) { 'internal error' }
+      it { expect(web_hook_log.success?).to be_falsey }
+    end
+  end
+end
diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb
index 9d4db1bfb52..53157c24477 100644
--- a/spec/models/hooks/web_hook_spec.rb
+++ b/spec/models/hooks/web_hook_spec.rb
@@ -1,89 +1,54 @@
 require 'spec_helper'
 
 describe WebHook, models: true do
-  describe "Validations" do
+  let(:hook) { build(:project_hook) }
+
+  describe 'associations' do
+    it { is_expected.to have_many(:web_hook_logs).dependent(:destroy) }
+  end
+
+  describe 'validations' do
     it { is_expected.to validate_presence_of(:url) }
 
     describe 'url' do
-      it { is_expected.to allow_value("http://example.com").for(:url) }
-      it { is_expected.to allow_value("https://example.com").for(:url) }
-      it { is_expected.to allow_value(" https://example.com ").for(:url) }
-      it { is_expected.to allow_value("http://test.com/api").for(:url) }
-      it { is_expected.to allow_value("http://test.com/api?key=abc").for(:url) }
-      it { is_expected.to allow_value("http://test.com/api?key=abc&type=def").for(:url) }
+      it { is_expected.to allow_value('http://example.com').for(:url) }
+      it { is_expected.to allow_value('https://example.com').for(:url) }
+      it { is_expected.to allow_value(' https://example.com ').for(:url) }
+      it { is_expected.to allow_value('http://test.com/api').for(:url) }
+      it { is_expected.to allow_value('http://test.com/api?key=abc').for(:url) }
+      it { is_expected.to allow_value('http://test.com/api?key=abc&type=def').for(:url) }
 
-      it { is_expected.not_to allow_value("example.com").for(:url) }
-      it { is_expected.not_to allow_value("ftp://example.com").for(:url) }
-      it { is_expected.not_to allow_value("herp-and-derp").for(:url) }
+      it { is_expected.not_to allow_value('example.com').for(:url) }
+      it { is_expected.not_to allow_value('ftp://example.com').for(:url) }
+      it { is_expected.not_to allow_value('herp-and-derp').for(:url) }
 
       it 'strips :url before saving it' do
-        hook = create(:project_hook, url: ' https://example.com ')
+        hook.url = ' https://example.com '
+        hook.save
 
         expect(hook.url).to eq('https://example.com')
       end
     end
   end
 
-  describe "execute" do
-    let(:project) { create(:empty_project) }
-    let(:project_hook) { create(:project_hook) }
-
-    before(:each) do
-      project.hooks << [project_hook]
-      @data = { before: 'oldrev', after: 'newrev', ref: 'ref' }
-
-      WebMock.stub_request(:post, project_hook.url)
-    end
-
-    context 'when token is defined' do
-      let(:project_hook) { create(:project_hook, :token) }
-
-      it 'POSTs to the webhook URL' do
-        project_hook.execute(@data, 'push_hooks')
-        expect(WebMock).to have_requested(:post, project_hook.url).with(
-          headers: { 'Content-Type' => 'application/json',
-                     'X-Gitlab-Event' => 'Push Hook',
-                     'X-Gitlab-Token' => project_hook.token }
-        ).once
-      end
-    end
-
-    it "POSTs to the webhook URL" do
-      project_hook.execute(@data, 'push_hooks')
-      expect(WebMock).to have_requested(:post, project_hook.url).with(
-        headers: { 'Content-Type' => 'application/json', 'X-Gitlab-Event' => 'Push Hook' }
-      ).once
-    end
-
-    it "POSTs the data as JSON" do
-      project_hook.execute(@data, 'push_hooks')
-      expect(WebMock).to have_requested(:post, project_hook.url).with(
-        headers: { 'Content-Type' => 'application/json', 'X-Gitlab-Event' => 'Push Hook' }
-      ).once
-    end
-
-    it "catches exceptions" do
-      expect(WebHook).to receive(:post).and_raise("Some HTTP Post error")
-
-      expect { project_hook.execute(@data, 'push_hooks') }.to raise_error(RuntimeError)
-    end
-
-    it "handles SSL exceptions" do
-      expect(WebHook).to receive(:post).and_raise(OpenSSL::SSL::SSLError.new('SSL error'))
+  describe 'execute' do
+    let(:data) { { key: 'value' } }
+    let(:hook_name) { 'project hook' }
 
-      expect(project_hook.execute(@data, 'push_hooks')).to eq([false, 'SSL error'])
+    before do
+      expect(WebHookService).to receive(:new).with(hook, data, hook_name).and_call_original
     end
 
-    it "handles 200 status code" do
-      WebMock.stub_request(:post, project_hook.url).to_return(status: 200, body: "Success")
+    it '#execute' do
+      expect_any_instance_of(WebHookService).to receive(:execute)
 
-      expect(project_hook.execute(@data, 'push_hooks')).to eq([200, 'Success'])
+      hook.execute(data, hook_name)
     end
 
-    it "handles 2xx status codes" do
-      WebMock.stub_request(:post, project_hook.url).to_return(status: 201, body: "Success")
+    it '#async_execute' do
+      expect_any_instance_of(WebHookService).to receive(:async_execute)
 
-      expect(project_hook.execute(@data, 'push_hooks')).to eq([201, 'Success'])
+      hook.async_execute(data, hook_name)
     end
   end
 end
diff --git a/spec/models/label_spec.rb b/spec/models/label_spec.rb
index 80ca19acdda..84867e3d96b 100644
--- a/spec/models/label_spec.rb
+++ b/spec/models/label_spec.rb
@@ -49,6 +49,23 @@ describe Label, models: true do
 
       expect(label.color).to eq('#abcdef')
     end
+
+    it 'uses default color if color is missing' do
+      label = described_class.new(color: nil)
+
+      expect(label.color).to be(Label::DEFAULT_COLOR)
+    end
+  end
+
+  describe '#text_color' do
+    it 'uses default color if color is missing' do
+      expect(LabelsHelper).to receive(:text_color_for_bg).with(Label::DEFAULT_COLOR).
+        and_return(spy)
+
+      label = described_class.new(color: nil)
+
+      label.text_color
+    end
   end
 
   describe '#title' do
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index da915c49d3c..712470d6bf5 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1243,7 +1243,7 @@ describe MergeRequest, models: true do
     end
   end
 
-  describe "#diff_sha_refs" do
+  describe "#diff_refs" do
     context "with diffs" do
       subject { create(:merge_request, :with_diffs) }
 
@@ -1252,7 +1252,7 @@ describe MergeRequest, models: true do
 
         expect_any_instance_of(Repository).not_to receive(:commit)
 
-        subject.diff_sha_refs
+        subject.diff_refs
       end
 
       it "returns expected diff_refs" do
@@ -1262,7 +1262,7 @@ describe MergeRequest, models: true do
           head_sha:  subject.merge_request_diff.head_commit_sha
         )
 
-        expect(subject.diff_sha_refs).to eq(expected_diff_refs)
+        expect(subject.diff_refs).to eq(expected_diff_refs)
       end
     end
   end
diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb
index 312302afdbb..ff5e7c350aa 100644
--- a/spec/models/namespace_spec.rb
+++ b/spec/models/namespace_spec.rb
@@ -238,8 +238,8 @@ describe Namespace, models: true do
     end
 
     context 'in sub-groups' do
-      let(:parent) { create(:namespace, path: 'parent') }
-      let(:child) { create(:namespace, parent: parent, path: 'child') }
+      let(:parent) { create(:group, path: 'parent') }
+      let(:child) { create(:group, parent: parent, path: 'child') }
       let!(:project) { create(:project_empty_repo, namespace: child) }
       let(:path_in_dir) { File.join(repository_storage_path, 'parent', 'child') }
       let(:deleted_path) { File.join('parent', "child+#{child.id}+deleted") }
diff --git a/spec/models/project_services/jira_service_spec.rb b/spec/models/project_services/jira_service_spec.rb
index 4bca0229e7a..349067e73ab 100644
--- a/spec/models/project_services/jira_service_spec.rb
+++ b/spec/models/project_services/jira_service_spec.rb
@@ -22,6 +22,42 @@ describe JiraService, models: true do
 
       it { is_expected.not_to validate_presence_of(:url) }
     end
+
+    context 'validating urls' do
+      let(:service) do
+        described_class.new(
+          project: create(:empty_project),
+          active: true,
+          username: 'username',
+          password: 'test',
+          project_key: 'TEST',
+          jira_issue_transition_id: 24,
+          url: 'http://jira.test.com'
+        )
+      end
+
+      it 'is valid when all fields have required values' do
+        expect(service).to be_valid
+      end
+
+      it 'is not valid when url is not a valid url' do
+        service.url = 'not valid'
+
+        expect(service).not_to be_valid
+      end
+
+      it 'is not valid when api url is not a valid url' do
+        service.api_url = 'not valid'
+
+        expect(service).not_to be_valid
+      end
+
+      it 'is valid when api url is a valid url' do
+        service.api_url = 'http://jira.test.com/api'
+
+        expect(service).to be_valid
+      end
+    end
   end
 
   describe '#reference_pattern' do
@@ -187,22 +223,29 @@ describe JiraService, models: true do
   describe '#test_settings' do
     let(:jira_service) do
       described_class.new(
+        project: create(:project),
         url: 'http://jira.example.com',
-        username: 'gitlab_jira_username',
-        password: 'gitlab_jira_password',
+        username: 'jira_username',
+        password: 'jira_password',
         project_key: 'GitLabProject'
       )
     end
-    let(:project_url) { 'http://gitlab_jira_username:gitlab_jira_password@jira.example.com/rest/api/2/project/GitLabProject' }
 
-    before do
+    def test_settings(api_url)
+      project_url = "http://jira_username:jira_password@#{api_url}/rest/api/2/project/GitLabProject"
+
       WebMock.stub_request(:get, project_url)
-    end
 
-    it 'tries to get JIRA project' do
       jira_service.test_settings
+    end
+
+    it 'tries to get JIRA project with URL when API URL not set' do
+      test_settings('jira.example.com')
+    end
 
-      expect(WebMock).to have_requested(:get, project_url)
+    it 'tries to get JIRA project with API URL if set' do
+      jira_service.update(api_url: 'http://jira.api.com')
+      test_settings('jira.api.com')
     end
   end
 
@@ -214,34 +257,75 @@ describe JiraService, models: true do
         @jira_service = JiraService.create!(
           project: project,
           properties: {
-            url: 'http://jira.example.com/rest/api/2',
+            url: 'http://jira.example.com/web',
             username: 'mic',
             password: "password"
           }
         )
       end
 
-      it "reset password if url changed" do
-        @jira_service.url = 'http://jira_edited.example.com/rest/api/2'
-        @jira_service.save
-        expect(@jira_service.password).to be_nil
+      context 'when only web url present' do
+        it 'reset password if url changed' do
+          @jira_service.url = 'http://jira_edited.example.com/rest/api/2'
+          @jira_service.save
+
+          expect(@jira_service.password).to be_nil
+        end
+
+        it 'reset password if url not changed but api url added' do
+          @jira_service.api_url = 'http://jira_edited.example.com/rest/api/2'
+          @jira_service.save
+
+          expect(@jira_service.password).to be_nil
+        end
       end
 
-      it "does not reset password if username changed" do
-        @jira_service.username = "some_name"
+      context 'when both web and api url present' do
+        before do
+          @jira_service.api_url = 'http://jira.example.com/rest/api/2'
+          @jira_service.password = 'password'
+
+          @jira_service.save
+        end
+        it 'reset password if api url changed' do
+          @jira_service.api_url = 'http://jira_edited.example.com/rest/api/2'
+          @jira_service.save
+
+          expect(@jira_service.password).to be_nil
+        end
+
+        it 'does not reset password if url changed' do
+          @jira_service.url = 'http://jira_edited.example.com/rweb'
+          @jira_service.save
+
+          expect(@jira_service.password).to eq("password")
+        end
+
+        it 'reset password if api url set to ""' do
+          @jira_service.api_url = ''
+          @jira_service.save
+
+          expect(@jira_service.password).to be_nil
+        end
+      end
+
+      it 'does not reset password if username changed' do
+        @jira_service.username = 'some_name'
         @jira_service.save
-        expect(@jira_service.password).to eq("password")
+
+        expect(@jira_service.password).to eq('password')
       end
 
-      it "does not reset password if new url is set together with password, even if it's the same password" do
+      it 'does not reset password if new url is set together with password, even if it\'s the same password' do
         @jira_service.url = 'http://jira_edited.example.com/rest/api/2'
         @jira_service.password = 'password'
         @jira_service.save
-        expect(@jira_service.password).to eq("password")
-        expect(@jira_service.url).to eq("http://jira_edited.example.com/rest/api/2")
+
+        expect(@jira_service.password).to eq('password')
+        expect(@jira_service.url).to eq('http://jira_edited.example.com/rest/api/2')
       end
 
-      it "resets password if url changed, even if setter called multiple times" do
+      it 'resets password if url changed, even if setter called multiple times' do
         @jira_service.url = 'http://jira1.example.com/rest/api/2'
         @jira_service.url = 'http://jira1.example.com/rest/api/2'
         @jira_service.save
@@ -249,7 +333,7 @@ describe JiraService, models: true do
       end
     end
 
-    context "when no password was previously set" do
+    context 'when no password was previously set' do
       before do
         @jira_service = JiraService.create(
           project: project,
@@ -260,26 +344,16 @@ describe JiraService, models: true do
         )
       end
 
-      it "saves password if new url is set together with password" do
+      it 'saves password if new url is set together with password' do
         @jira_service.url = 'http://jira_edited.example.com/rest/api/2'
         @jira_service.password = 'password'
         @jira_service.save
-        expect(@jira_service.password).to eq("password")
-        expect(@jira_service.url).to eq("http://jira_edited.example.com/rest/api/2")
+        expect(@jira_service.password).to eq('password')
+        expect(@jira_service.url).to eq('http://jira_edited.example.com/rest/api/2')
       end
     end
   end
 
-  describe "Validations" do
-    context "active" do
-      before do
-        subject.active = true
-      end
-
-      it { is_expected.to validate_presence_of :url }
-    end
-  end
-
   describe 'description and title' do
     let(:project) { create(:empty_project) }
 
@@ -321,9 +395,10 @@ describe JiraService, models: true do
     context 'when gitlab.yml was initialized' do
       before do
         settings = {
-          "jira" => {
-            "title" => "Jira",
-            "url" => "http://jira.sample/projects/project_a"
+          'jira' => {
+            'title' => 'Jira',
+            'url' => 'http://jira.sample/projects/project_a',
+            'api_url' => 'http://jira.sample/api'
           }
         }
         allow(Gitlab.config).to receive(:issues_tracker).and_return(settings)
@@ -335,8 +410,9 @@ describe JiraService, models: true do
       end
 
       it 'is prepopulated with the settings' do
-        expect(@service.properties["title"]).to eq('Jira')
-        expect(@service.properties["url"]).to eq('http://jira.sample/projects/project_a')
+        expect(@service.properties['title']).to eq('Jira')
+        expect(@service.properties['url']).to eq('http://jira.sample/projects/project_a')
+        expect(@service.properties['api_url']).to eq('http://jira.sample/api')
       end
     end
   end
diff --git a/spec/models/project_services/kubernetes_service_spec.rb b/spec/models/project_services/kubernetes_service_spec.rb
index c1c2f2a7219..0dcf4a4b5d6 100644
--- a/spec/models/project_services/kubernetes_service_spec.rb
+++ b/spec/models/project_services/kubernetes_service_spec.rb
@@ -13,7 +13,7 @@ describe KubernetesService, models: true, caching: true do
   let(:discovery_url) { service.api_url + '/api/v1' }
   let(:discovery_response) { { body: kube_discovery_body.to_json } }
 
-  let(:pods_url) { service.api_url + "/api/v1/namespaces/#{service.namespace}/pods" }
+  let(:pods_url) { service.api_url + "/api/v1/namespaces/#{service.actual_namespace}/pods" }
   let(:pods_response) { { body: kube_pods_body(kube_pod).to_json } }
 
   def stub_kubeclient_discover
@@ -100,7 +100,35 @@ describe KubernetesService, models: true, caching: true do
 
       it 'sets the namespace to the default' do
         expect(kube_namespace).not_to be_nil
-        expect(kube_namespace[:placeholder]).to match(/\A#{Gitlab::Regex::PATH_REGEX_STR}-\d+\z/)
+        expect(kube_namespace[:placeholder]).to match(/\A#{Gitlab::PathRegex::PATH_REGEX_STR}-\d+\z/)
+      end
+    end
+  end
+
+  describe '#actual_namespace' do
+    subject { service.actual_namespace }
+
+    it "returns the default namespace" do
+      is_expected.to eq(service.send(:default_namespace))
+    end
+    
+    context 'when namespace is specified' do
+      before do
+        service.namespace = 'my-namespace'
+      end
+
+      it "returns the user-namespace" do
+        is_expected.to eq('my-namespace')
+      end
+    end
+
+    context 'when service is not assigned to project' do
+      before do
+        service.project = nil
+      end
+
+      it "does not return namespace" do
+        is_expected.to be_nil
       end
     end
   end
@@ -187,13 +215,14 @@ describe KubernetesService, models: true, caching: true do
         kube_namespace = subject.predefined_variables.find { |h| h[:key] == 'KUBE_NAMESPACE' }
 
         expect(kube_namespace).not_to be_nil
-        expect(kube_namespace[:value]).to match(/\A#{Gitlab::Regex::PATH_REGEX_STR}-\d+\z/)
+        expect(kube_namespace[:value]).to match(/\A#{Gitlab::PathRegex::PATH_REGEX_STR}-\d+\z/)
       end
     end
   end
 
   describe '#terminals' do
     let(:environment) { build(:environment, project: project, name: "env", slug: "env-000000") }
+
     subject { service.terminals(environment) }
 
     context 'with invalid pods' do
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 6a15830a15c..aabdac4bb75 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -440,6 +440,22 @@ describe User, models: true do
     end
   end
 
+  describe 'ensure incoming email token' do
+    it 'has incoming email token' do
+      user = create(:user)
+      expect(user.incoming_email_token).not_to be_blank
+    end
+  end
+
+  describe 'rss token' do
+    it 'ensures an rss token on read' do
+      user = create(:user, rss_token: nil)
+      rss_token = user.rss_token
+      expect(rss_token).not_to be_blank
+      expect(user.reload.rss_token).to eq rss_token
+    end
+  end
+
   describe '#recently_sent_password_reset?' do
     it 'is false when reset_password_sent_at is nil' do
       user = build_stubbed(:user, reset_password_sent_at: nil)