Merge remote-tracking branch 'upstream/master' into 28717-additional-metrics-review-branch

# Conflicts:
#	app/models/project_services/prometheus_service.rb
#	app/views/projects/services/_form.html.haml

19 files changed, 540 insertions, 286 deletions

diff --git a/spec/models/abuse_report_spec.rb b/spec/models/abuse_report_spec.rb
index ced93c8f762..90aec2b45e6 100644
--- a/spec/models/abuse_report_spec.rb
+++ b/spec/models/abuse_report_spec.rb
@@ -28,9 +28,7 @@ RSpec.describe AbuseReport, type: :model do
     end
 
     it 'lets a worker delete the user' do
-      expect(DeleteUserWorker).to receive(:perform_async).with(user.id, subject.user.id,
-                                                              delete_solo_owned_groups: true,
-                                                              hard_delete: true)
+      expect(DeleteUserWorker).to receive(:perform_async).with(user.id, subject.user.id, hard_delete: true)
 
       subject.remove_user(deleted_by: user)
     end
diff --git a/spec/models/blob_viewer/base_spec.rb b/spec/models/blob_viewer/base_spec.rb
index 92fbf64a6b7..d56379eb59d 100644
--- a/spec/models/blob_viewer/base_spec.rb
+++ b/spec/models/blob_viewer/base_spec.rb
@@ -11,8 +11,8 @@ describe BlobViewer::Base, model: true do
 
       self.extensions = %w(pdf)
       self.binary = true
-      self.overridable_max_size = 1.megabyte
-      self.max_size = 5.megabytes
+      self.collapse_limit = 1.megabyte
+      self.size_limit = 5.megabytes
     end
   end
 
@@ -69,77 +69,49 @@ describe BlobViewer::Base, model: true do
     end
   end
 
-  describe '#exceeds_overridable_max_size?' do
-    context 'when the blob size is larger than the overridable max size' do
+  describe '#collapsed?' do
+    context 'when the blob size is larger than the collapse limit' do
       let(:blob) { fake_blob(path: 'file.pdf', size: 2.megabytes) }
 
       it 'returns true' do
-        expect(viewer.exceeds_overridable_max_size?).to be_truthy
+        expect(viewer.collapsed?).to be_truthy
       end
     end
 
-    context 'when the blob size is smaller than the overridable max size' do
+    context 'when the blob size is smaller than the collapse limit' do
       let(:blob) { fake_blob(path: 'file.pdf', size: 10.kilobytes) }
 
       it 'returns false' do
-        expect(viewer.exceeds_overridable_max_size?).to be_falsey
+        expect(viewer.collapsed?).to be_falsey
       end
     end
   end
 
-  describe '#exceeds_max_size?' do
-    context 'when the blob size is larger than the max size' do
+  describe '#too_large?' do
+    context 'when the blob size is larger than the size limit' do
       let(:blob) { fake_blob(path: 'file.pdf', size: 10.megabytes) }
 
       it 'returns true' do
-        expect(viewer.exceeds_max_size?).to be_truthy
+        expect(viewer.too_large?).to be_truthy
       end
     end
 
-    context 'when the blob size is smaller than the max size' do
+    context 'when the blob size is smaller than the size limit' do
       let(:blob) { fake_blob(path: 'file.pdf', size: 2.megabytes) }
 
       it 'returns false' do
-        expect(viewer.exceeds_max_size?).to be_falsey
-      end
-    end
-  end
-
-  describe '#can_override_max_size?' do
-    context 'when the blob size is larger than the overridable max size' do
-      context 'when the blob size is larger than the max size' do
-        let(:blob) { fake_blob(path: 'file.pdf', size: 10.megabytes) }
-
-        it 'returns false' do
-          expect(viewer.can_override_max_size?).to be_falsey
-        end
-      end
-
-      context 'when the blob size is smaller than the max size' do
-        let(:blob) { fake_blob(path: 'file.pdf', size: 2.megabytes) }
-
-        it 'returns true' do
-          expect(viewer.can_override_max_size?).to be_truthy
-        end
-      end
-    end
-
-    context 'when the blob size is smaller than the overridable max size' do
-      let(:blob) { fake_blob(path: 'file.pdf', size: 10.kilobytes) }
-
-      it 'returns false' do
-        expect(viewer.can_override_max_size?).to be_falsey
+        expect(viewer.too_large?).to be_falsey
       end
     end
   end
 
   describe '#render_error' do
-    context 'when the max size is overridden' do
+    context 'when expanded' do
       before do
-        viewer.override_max_size = true
+        viewer.expanded = true
       end
 
-      context 'when the blob size is larger than the max size' do
+      context 'when the blob size is larger than the size limit' do
         let(:blob) { fake_blob(path: 'file.pdf', size: 10.megabytes) }
 
         it 'returns :too_large' do
@@ -147,7 +119,7 @@ describe BlobViewer::Base, model: true do
         end
       end
 
-      context 'when the blob size is smaller than the max size' do
+      context 'when the blob size is smaller than the size limit' do
         let(:blob) { fake_blob(path: 'file.pdf', size: 2.megabytes) }
 
         it 'returns nil' do
@@ -156,16 +128,16 @@ describe BlobViewer::Base, model: true do
       end
     end
 
-    context 'when the max size is not overridden' do
-      context 'when the blob size is larger than the overridable max size' do
+    context 'when not expanded' do
+      context 'when the blob size is larger than the collapse limit' do
         let(:blob) { fake_blob(path: 'file.pdf', size: 2.megabytes) }
 
-        it 'returns :too_large' do
-          expect(viewer.render_error).to eq(:too_large)
+        it 'returns :collapsed' do
+          expect(viewer.render_error).to eq(:collapsed)
         end
       end
 
-      context 'when the blob size is smaller than the overridable max size' do
+      context 'when the blob size is smaller than the collapse limit' do
         let(:blob) { fake_blob(path: 'file.pdf', size: 10.kilobytes) }
 
         it 'returns nil' do
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index e971b4bc3f9..b0716e04d3d 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -427,6 +427,42 @@ describe Ci::Build, :models do
       end
     end
 
+    describe '#environment_url' do
+      subject { job.environment_url }
+
+      context 'when yaml environment uses $CI_COMMIT_REF_NAME' do
+        let(:job) do
+          create(:ci_build,
+                 ref: 'master',
+                 options: { environment: { url: 'http://review/$CI_COMMIT_REF_NAME' } })
+        end
+
+        it { is_expected.to eq('http://review/master') }
+      end
+
+      context 'when yaml environment uses yaml_variables containing symbol keys' do
+        let(:job) do
+          create(:ci_build,
+                 yaml_variables: [{ key: :APP_HOST, value: 'host' }],
+                 options: { environment: { url: 'http://review/$APP_HOST' } })
+        end
+
+        it { is_expected.to eq('http://review/host') }
+      end
+
+      context 'when yaml environment does not have url' do
+        let(:job) { create(:ci_build, environment: 'staging') }
+
+        let!(:environment) do
+          create(:environment, project: job.project, name: job.environment)
+        end
+
+        it 'returns the external_url from persisted environment' do
+          is_expected.to eq(environment.external_url)
+        end
+      end
+    end
+
     describe '#starts_environment?' do
       subject { build.starts_environment? }
 
@@ -918,6 +954,10 @@ describe Ci::Build, :models do
 
       it { is_expected.to eq(environment) }
     end
+
+    context 'when there is no environment' do
+      it { is_expected.to be_nil }
+    end
   end
 
   describe '#play' do
@@ -1139,6 +1179,7 @@ describe Ci::Build, :models do
         { key: 'CI_PROJECT_ID', value: project.id.to_s, public: true },
         { key: 'CI_PROJECT_NAME', value: project.path, public: true },
         { key: 'CI_PROJECT_PATH', value: project.full_path, public: true },
+        { key: 'CI_PROJECT_PATH_SLUG', value: project.full_path.parameterize, public: true },
         { key: 'CI_PROJECT_NAMESPACE', value: project.namespace.full_path, public: true },
         { key: 'CI_PROJECT_URL', value: project.web_url, public: true },
         { key: 'CI_PIPELINE_ID', value: pipeline.id.to_s, public: true },
@@ -1176,11 +1217,6 @@ describe Ci::Build, :models do
     end
 
     context 'when build has an environment' do
-      before do
-        build.update(environment: 'production')
-        create(:environment, project: build.project, name: 'production', slug: 'prod-slug')
-      end
-
       let(:environment_variables) do
         [
           { key: 'CI_ENVIRONMENT_NAME', value: 'production', public: true },
@@ -1188,7 +1224,56 @@ describe Ci::Build, :models do
         ]
       end
 
-      it { environment_variables.each { |v| is_expected.to include(v) } }
+      let!(:environment) do
+        create(:environment,
+          project: build.project,
+          name: 'production',
+          slug: 'prod-slug',
+          external_url: '')
+      end
+
+      before do
+        build.update(environment: 'production')
+      end
+
+      shared_examples 'containing environment variables' do
+        it { environment_variables.each { |v| is_expected.to include(v) } }
+      end
+
+      context 'when no URL was set' do
+        it_behaves_like 'containing environment variables'
+
+        it 'does not have CI_ENVIRONMENT_URL' do
+          keys = subject.map { |var| var[:key] }
+
+          expect(keys).not_to include('CI_ENVIRONMENT_URL')
+        end
+      end
+
+      context 'when an URL was set' do
+        let(:url) { 'http://host/test' }
+
+        before do
+          environment_variables <<
+            { key: 'CI_ENVIRONMENT_URL', value: url, public: true }
+        end
+
+        context 'when the URL was set from the job' do
+          before do
+            build.update(options: { environment: { url: 'http://host/$CI_JOB_NAME' } })
+          end
+
+          it_behaves_like 'containing environment variables'
+        end
+
+        context 'when the URL was not set from the job, but environment' do
+          before do
+            environment.update(external_url: url)
+          end
+
+          it_behaves_like 'containing environment variables'
+        end
+      end
     end
 
     context 'when build started manually' do
@@ -1215,16 +1300,49 @@ describe Ci::Build, :models do
       it { is_expected.to include(tag_variable) }
     end
 
-    context 'when secure variable is defined' do
-      let(:secure_variable) do
+    context 'when secret variable is defined' do
+      let(:secret_variable) do
         { key: 'SECRET_KEY', value: 'secret_value', public: false }
       end
 
       before do
-        build.project.variables << Ci::Variable.new(key: 'SECRET_KEY', value: 'secret_value')
+        create(:ci_variable,
+               secret_variable.slice(:key, :value).merge(project: project))
+      end
+
+      it { is_expected.to include(secret_variable) }
+    end
+
+    context 'when protected variable is defined' do
+      let(:protected_variable) do
+        { key: 'PROTECTED_KEY', value: 'protected_value', public: false }
+      end
+
+      before do
+        create(:ci_variable,
+               :protected,
+               protected_variable.slice(:key, :value).merge(project: project))
+      end
+
+      context 'when the branch is protected' do
+        before do
+          create(:protected_branch, project: build.project, name: build.ref)
+        end
+
+        it { is_expected.to include(protected_variable) }
       end
 
-      it { is_expected.to include(secure_variable) }
+      context 'when the tag is protected' do
+        before do
+          create(:protected_tag, project: build.project, name: build.ref)
+        end
+
+        it { is_expected.to include(protected_variable) }
+      end
+
+      context 'when the ref is not protected' do
+        it { is_expected.not_to include(protected_variable) }
+      end
     end
 
     context 'when build is for triggers' do
@@ -1346,15 +1464,30 @@ describe Ci::Build, :models do
     end
 
     context 'returns variables in valid order' do
+      let(:build_pre_var) { { key: 'build', value: 'value' } }
+      let(:project_pre_var) { { key: 'project', value: 'value' } }
+      let(:pipeline_pre_var) { { key: 'pipeline', value: 'value' } }
+      let(:build_yaml_var) { { key: 'yaml', value: 'value' } }
+
       before do
-        allow(build).to receive(:predefined_variables) { ['predefined'] }
-        allow(project).to receive(:predefined_variables) { ['project'] }
-        allow(pipeline).to receive(:predefined_variables) { ['pipeline'] }
-        allow(build).to receive(:yaml_variables) { ['yaml'] }
-        allow(project).to receive(:secret_variables) { ['secret'] }
+        allow(build).to receive(:predefined_variables) { [build_pre_var] }
+        allow(project).to receive(:predefined_variables) { [project_pre_var] }
+        allow(pipeline).to receive(:predefined_variables) { [pipeline_pre_var] }
+        allow(build).to receive(:yaml_variables) { [build_yaml_var] }
+
+        allow(project).to receive(:secret_variables_for).with(build.ref) do
+          [create(:ci_variable, key: 'secret', value: 'value')]
+        end
       end
 
-      it { is_expected.to eq(%w[predefined project pipeline yaml secret]) }
+      it do
+        is_expected.to eq(
+          [build_pre_var,
+           project_pre_var,
+           pipeline_pre_var,
+           build_yaml_var,
+           { key: 'secret', value: 'value', public: false }])
+      end
     end
   end
 
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index c8023dc13b1..ae1b01b76ab 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -21,13 +21,35 @@ describe Ci::Pipeline, models: true do
   it { is_expected.to have_many(:auto_canceled_pipelines) }
   it { is_expected.to have_many(:auto_canceled_jobs) }
 
-  it { is_expected.to validate_presence_of :sha }
-  it { is_expected.to validate_presence_of :status }
+  it { is_expected.to validate_presence_of(:sha) }
+  it { is_expected.to validate_presence_of(:status) }
 
   it { is_expected.to respond_to :git_author_name }
   it { is_expected.to respond_to :git_author_email }
   it { is_expected.to respond_to :short_sha }
 
+  describe '#source' do
+    context 'when creating new pipeline' do
+      let(:pipeline) do
+        build(:ci_empty_pipeline, status: :created, project: project, source: nil)
+      end
+
+      it "prevents from creating an object" do
+        expect(pipeline).not_to be_valid
+      end
+    end
+
+    context 'when updating existing pipeline' do
+      before do
+        pipeline.update_attribute(:source, nil)
+      end
+
+      it "object is valid" do
+        expect(pipeline).to be_valid
+      end
+    end
+  end
+
   describe '#block' do
     it 'changes pipeline status to manual' do
       expect(pipeline.block).to be true
diff --git a/spec/models/ci/variable_spec.rb b/spec/models/ci/variable_spec.rb
index fe8c52d5353..077b10227d7 100644
--- a/spec/models/ci/variable_spec.rb
+++ b/spec/models/ci/variable_spec.rb
@@ -12,11 +12,33 @@ describe Ci::Variable, models: true do
   it { is_expected.not_to allow_value('foo bar').for(:key) }
   it { is_expected.not_to allow_value('foo/bar').for(:key) }
 
-  before :each do
-    subject.value = secret_value
+  describe '.unprotected' do
+    subject { described_class.unprotected }
+
+    context 'when variable is protected' do
+      before do
+        create(:ci_variable, :protected)
+      end
+
+      it 'returns nothing' do
+        is_expected.to be_empty
+      end
+    end
+
+    context 'when variable is not protected' do
+      let(:variable) { create(:ci_variable, protected: false) }
+
+      it 'returns the variable' do
+        is_expected.to contain_exactly(variable)
+      end
+    end
   end
 
   describe '#value' do
+    before do
+      subject.value = secret_value
+    end
+
     it 'stores the encrypted value' do
       expect(subject.encrypted_value).not_to be_nil
     end
@@ -36,4 +58,11 @@ describe Ci::Variable, models: true do
         to raise_error(OpenSSL::Cipher::CipherError, 'bad decrypt')
     end
   end
+
+  describe '#to_runner_variable' do
+    it 'returns a hash for the runner' do
+      expect(subject.to_runner_variable)
+        .to eq(key: subject.key, value: subject.value, public: false)
+    end
+  end
 end
diff --git a/spec/models/concerns/editable_spec.rb b/spec/models/concerns/editable_spec.rb
new file mode 100644
index 00000000000..cd73af3b480
--- /dev/null
+++ b/spec/models/concerns/editable_spec.rb
@@ -0,0 +1,11 @@
+require 'spec_helper'
+
+describe Editable do
+  describe '#is_edited?' do
+    let(:issue) { create(:issue, last_edited_at: nil) }
+    let(:edited_issue) { create(:issue, created_at: 3.days.ago, last_edited_at: 2.days.ago) }
+
+    it { expect(issue.is_edited?).to eq(false) }
+    it { expect(edited_issue.is_edited?).to eq(true) }
+  end
+end
diff --git a/spec/models/deployment_spec.rb b/spec/models/deployment_spec.rb
index bbb7dbf0922..d15ff9e5ffd 100644
--- a/spec/models/deployment_spec.rb
+++ b/spec/models/deployment_spec.rb
@@ -16,6 +16,19 @@ describe Deployment, models: true do
   it { is_expected.to validate_presence_of(:ref) }
   it { is_expected.to validate_presence_of(:sha) }
 
+  describe 'after_create callbacks' do
+    let(:environment) { create(:environment) }
+    let(:store) { Gitlab::EtagCaching::Store.new }
+
+    it 'invalidates the environment etag cache' do
+      old_value = store.get(environment.etag_cache_key)
+
+      create(:deployment, environment: environment)
+
+      expect(store.get(environment.etag_cache_key)).not_to eq(old_value)
+    end
+  end
+
   describe '#includes_commit?' do
     let(:project)     { create(:project, :repository) }
     let(:environment) { create(:environment, project: project) }
diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb
index 96f075d4f7d..297c2108dc2 100644
--- a/spec/models/diff_note_spec.rb
+++ b/spec/models/diff_note_spec.rb
@@ -160,12 +160,6 @@ describe DiffNote, models: true do
       context "when noteable is a commit" do
         let(:diff_note) { create(:diff_note_on_commit, project: project, position: position) }
 
-        it "doesn't use the DiffPositionUpdateService" do
-          expect(Notes::DiffPositionUpdateService).not_to receive(:new)
-
-          diff_note
-        end
-
         it "doesn't update the position" do
           diff_note
 
@@ -178,12 +172,6 @@ describe DiffNote, models: true do
         let(:diff_note) { create(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) }
 
         context "when the note is active" do
-          it "doesn't use the DiffPositionUpdateService" do
-            expect(Notes::DiffPositionUpdateService).not_to receive(:new)
-
-            diff_note
-          end
-
           it "doesn't update the position" do
             diff_note
 
@@ -197,18 +185,11 @@ describe DiffNote, models: true do
             allow(merge_request).to receive(:diff_refs).and_return(commit.diff_refs)
           end
 
-          it "uses the DiffPositionUpdateService" do
-            service = instance_double("Notes::DiffPositionUpdateService")
-            expect(Notes::DiffPositionUpdateService).to receive(:new).with(
-              project,
-              nil,
-              old_diff_refs: position.diff_refs,
-              new_diff_refs: commit.diff_refs,
-              paths: [path]
-            ).and_return(service)
-            expect(service).to receive(:execute)
-
+          it "updates the position" do
             diff_note
+
+            expect(diff_note.original_position).to eq(position)
+            expect(diff_note.position).not_to eq(position)
           end
         end
       end
diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb
index c8709409cea..f711065a0ab 100644
--- a/spec/models/environment_spec.rb
+++ b/spec/models/environment_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 describe Environment, models: true do
-  let(:project) { create(:empty_project) }
+  set(:project) { create(:empty_project) }
   subject(:environment) { create(:environment, project: project) }
 
   it { is_expected.to belong_to(:project) }
@@ -34,6 +34,26 @@ describe Environment, models: true do
     end
   end
 
+  describe 'state machine' do
+    it 'invalidates the cache after a change' do
+      expect(environment).to receive(:expire_etag_cache)
+
+      environment.stop
+    end
+  end
+
+  describe '#expire_etag_cache' do
+    let(:store) { Gitlab::EtagCaching::Store.new }
+
+    it 'changes the cached value' do
+      old_value = store.get(environment.etag_cache_key)
+
+      environment.stop
+
+      expect(store.get(environment.etag_cache_key)).not_to eq(old_value)
+    end
+  end
+
   describe '#nullify_external_url' do
     it 'replaces a blank url with nil' do
       env = build(:environment, external_url: "")
@@ -227,7 +247,10 @@ describe Environment, models: true do
 
       context 'when user is allowed to stop environment' do
         before do
-          project.add_master(user)
+          project.add_developer(user)
+
+          create(:protected_branch, :developers_can_merge,
+                 name: 'master', project: project)
         end
 
         context 'when action did not yet finish' do
diff --git a/spec/models/forked_project_link_spec.rb b/spec/models/forked_project_link_spec.rb
index 454550c9710..6e8d43f988c 100644
--- a/spec/models/forked_project_link_spec.rb
+++ b/spec/models/forked_project_link_spec.rb
@@ -2,8 +2,8 @@ require 'spec_helper'
 
 describe ForkedProjectLink, "add link on fork" do
   let(:project_from) { create(:project, :repository) }
-  let(:namespace) { create(:namespace) }
-  let(:user) { create(:user, namespace: namespace) }
+  let(:user) { create(:user) }
+  let(:namespace) { user.namespace }
 
   before do
     create(:project_member, :reporter, user: user, project: project_from)
diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb
index 7c40cfd8253..f1e2a2cc518 100644
--- a/spec/models/key_spec.rb
+++ b/spec/models/key_spec.rb
@@ -66,14 +66,16 @@ describe Key, models: true do
     end
 
     it "does not accept the exact same key twice" do
-      create(:key, user: user)
-      expect(build(:key, user: user)).not_to be_valid
+      first_key = create(:key, user: user)
+
+      expect(build(:key, user: user, key: first_key.key)).not_to be_valid
     end
 
     it "does not accept a duplicate key with a different comment" do
-      create(:key, user: user)
-      duplicate = build(:key, user: user)
+      first_key = create(:key, user: user)
+      duplicate = build(:key, user: user, key: first_key.key)
       duplicate.key << ' extra comment'
+
       expect(duplicate).not_to be_valid
     end
   end
diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb
index 0a10ee01506..ed9fde57bf7 100644
--- a/spec/models/merge_request_diff_spec.rb
+++ b/spec/models/merge_request_diff_spec.rb
@@ -139,4 +139,15 @@ describe MergeRequestDiff, models: true do
       expect(subject.commits_count).to eq 2
     end
   end
+
+  describe '#utf8_st_diffs' do
+    it 'does not raise error when a hash value is in binary' do
+      subject.st_diffs = [
+        { diff: "\0" },
+        { diff: "\x05\x00\x68\x65\x6c\x6c\x6f" }
+      ]
+
+      expect { subject.utf8_st_diffs }.not_to raise_error
+    end
+  end
 end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 712470d6bf5..060754fab63 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -238,10 +238,10 @@ describe MergeRequest, models: true do
     end
 
     context 'when there are no MR diffs' do
-      it 'delegates to the compare object, setting no_collapse: true' do
+      it 'delegates to the compare object, setting expanded: true' do
         merge_request.compare = double(:compare)
 
-        expect(merge_request.compare).to receive(:diffs).with(options.merge(no_collapse: true))
+        expect(merge_request.compare).to receive(:diffs).with(options.merge(expanded: true))
 
         merge_request.diffs(options)
       end
@@ -1178,7 +1178,7 @@ describe MergeRequest, models: true do
   end
 
   describe "#reload_diff" do
-    let(:note) { create(:diff_note_on_merge_request, project: subject.project, noteable: subject) }
+    let(:discussion) { create(:diff_note_on_merge_request, project: subject.project, noteable: subject).to_discussion }
 
     let(:commit) { subject.project.commit(sample_commit.id) }
 
@@ -1197,7 +1197,7 @@ describe MergeRequest, models: true do
       subject.reload_diff
     end
 
-    it "updates diff note positions" do
+    it "updates diff discussion positions" do
       old_diff_refs = subject.diff_refs
 
       # Update merge_request_diff so that #diff_refs will return commit.diff_refs
@@ -1211,15 +1211,15 @@ describe MergeRequest, models: true do
         subject.merge_request_diff(true)
       end
 
-      expect(Notes::DiffPositionUpdateService).to receive(:new).with(
+      expect(Discussions::UpdateDiffPositionService).to receive(:new).with(
         subject.project,
         subject.author,
         old_diff_refs: old_diff_refs,
         new_diff_refs: commit.diff_refs,
-        paths: note.position.paths
+        paths: discussion.position.paths
       ).and_call_original
 
-      expect_any_instance_of(Notes::DiffPositionUpdateService).to receive(:execute).with(note)
+      expect_any_instance_of(Discussions::UpdateDiffPositionService).to receive(:execute).with(discussion).and_call_original
       expect_any_instance_of(DiffNote).to receive(:save).once
 
       subject.reload_diff(subject.author)
diff --git a/spec/models/pages_domain_spec.rb b/spec/models/pages_domain_spec.rb
index c6c45d78990..f9d060d4e0e 100644
--- a/spec/models/pages_domain_spec.rb
+++ b/spec/models/pages_domain_spec.rb
@@ -6,7 +6,7 @@ describe PagesDomain, models: true do
   end
 
   describe 'validate domain' do
-    subject { build(:pages_domain, domain: domain) }
+    subject(:pages_domain) { build(:pages_domain, domain: domain) }
 
     context 'is unique' do
       let(:domain) { 'my.domain.com' }
@@ -14,36 +14,25 @@ describe PagesDomain, models: true do
       it { is_expected.to validate_uniqueness_of(:domain) }
     end
 
-    context 'valid domain' do
-      let(:domain) { 'my.domain.com' }
-
-      it { is_expected.to be_valid }
-    end
-
-    context 'valid hexadecimal-looking domain' do
-      let(:domain) { '0x12345.com'}
-
-      it { is_expected.to be_valid }
-    end
-
-    context 'no domain' do
-      let(:domain) { nil }
-
-      it { is_expected.not_to be_valid }
-    end
-
-    context 'invalid domain' do
-      let(:domain) { '0123123' }
-
-      it { is_expected.not_to be_valid }
-    end
-
-    context 'domain from .example.com' do
-      let(:domain) { 'my.domain.com' }
-
-      before { allow(Settings.pages).to receive(:host).and_return('domain.com') }
-
-      it { is_expected.not_to be_valid }
+    {
+      'my.domain.com'    => true,
+      '123.456.789'      => true,
+      '0x12345.com'      => true,
+      '0123123'          => true,
+      '_foo.com'         => false,
+      'reserved.com'     => false,
+      'a.reserved.com'   => false,
+      nil                => false
+    }.each do |value, validity|
+      context "domain #{value.inspect} validity" do
+        before do
+          allow(Settings.pages).to receive(:host).and_return('reserved.com')
+        end
+
+        let(:domain) { value }
+
+        it { expect(pages_domain.valid?).to eq(validity) }
+      end
     end
   end
 
diff --git a/spec/models/project_services/jira_service_spec.rb b/spec/models/project_services/jira_service_spec.rb
index 349067e73ab..0ee050196e4 100644
--- a/spec/models/project_services/jira_service_spec.rb
+++ b/spec/models/project_services/jira_service_spec.rb
@@ -69,41 +69,6 @@ describe JiraService, models: true do
     end
   end
 
-  describe '#can_test?' do
-    let(:jira_service) { described_class.new }
-
-    it 'returns false if username is blank' do
-      allow(jira_service).to receive_messages(
-        url: 'http://jira.example.com',
-        username: '',
-        password: '12345678'
-      )
-
-      expect(jira_service.can_test?).to be_falsy
-    end
-
-    it 'returns false if password is blank' do
-      allow(jira_service).to receive_messages(
-        url: 'http://jira.example.com',
-        username: 'tester',
-        password: ''
-      )
-
-      expect(jira_service.can_test?).to be_falsy
-    end
-
-    it 'returns true if password and username are present' do
-      jira_service = described_class.new
-      allow(jira_service).to receive_messages(
-        url: 'http://jira.example.com',
-        username: 'tester',
-        password: '12345678'
-      )
-
-      expect(jira_service.can_test?).to be_truthy
-    end
-  end
-
   describe '#close_issue' do
     let(:custom_base_url) { 'http://custom_url' }
     let(:user)    { create(:user) }
@@ -133,6 +98,7 @@ describe JiraService, models: true do
       allow(JIRA::Resource::Issue).to receive(:find).and_return(open_issue, closed_issue)
 
       allow_any_instance_of(JIRA::Resource::Issue).to receive(:key).and_return("JIRA-123")
+      allow(JIRA::Resource::Remotelink).to receive(:all).and_return([])
 
       @jira_service.save
 
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 36575acf671..3ed52d42f86 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -50,7 +50,7 @@ describe Project, models: true do
     it { is_expected.to have_one(:external_wiki_service).dependent(:destroy) }
     it { is_expected.to have_one(:project_feature).dependent(:destroy) }
     it { is_expected.to have_one(:statistics).class_name('ProjectStatistics').dependent(:delete) }
-    it { is_expected.to have_one(:import_data).class_name('ProjectImportData').dependent(:destroy) }
+    it { is_expected.to have_one(:import_data).class_name('ProjectImportData').dependent(:delete) }
     it { is_expected.to have_one(:last_event).class_name('Event') }
     it { is_expected.to have_one(:forked_from_project).through(:forked_project_link) }
     it { is_expected.to have_many(:commit_statuses) }
@@ -948,6 +948,20 @@ describe Project, models: true do
     end
   end
 
+  describe '.starred_by' do
+    it 'returns only projects starred by the given user' do
+      user1 = create(:user)
+      user2 = create(:user)
+      project1 = create(:empty_project)
+      project2 = create(:empty_project)
+      create(:empty_project)
+      user1.toggle_star(project1)
+      user2.toggle_star(project2)
+
+      expect(Project.starred_by(user1)).to contain_exactly(project1)
+    end
+  end
+
   describe '.visible_to_user' do
     let!(:project) { create(:empty_project, :private) }
     let!(:user)    { create(:user) }
@@ -1432,16 +1446,13 @@ describe Project, models: true do
   end
 
   describe 'Project import job' do
-    let(:project) { create(:empty_project) }
-    let(:mirror) { false }
+    let(:project) { create(:empty_project, import_url: generate(:url)) }
 
     before do
       allow_any_instance_of(Gitlab::Shell).to receive(:import_repository)
         .with(project.repository_storage_path, project.path_with_namespace, project.import_url)
         .and_return(true)
 
-      allow(project).to receive(:repository_exists?).and_return(true)
-
       expect_any_instance_of(Repository).to receive(:after_import)
         .and_call_original
     end
@@ -1449,8 +1460,7 @@ describe Project, models: true do
     it 'imports a project' do
       expect_any_instance_of(RepositoryImportWorker).to receive(:perform).and_call_original
 
-      project.import_start
-      project.add_import_job
+      project.import_schedule
 
       expect(project.reload.import_status).to eq('finished')
     end
@@ -1537,7 +1547,7 @@ describe Project, models: true do
 
   describe '#add_import_job' do
     context 'forked' do
-      let(:forked_project_link) { create(:forked_project_link) }
+      let(:forked_project_link) { create(:forked_project_link, :forked_to_empty_project) }
       let(:forked_from_project) { forked_project_link.forked_from_project }
       let(:project) { forked_project_link.forked_to_project }
 
@@ -1551,9 +1561,9 @@ describe Project, models: true do
     end
 
     context 'not forked' do
-      let(:project) { create(:empty_project) }
-
       it 'schedules a RepositoryImportWorker job' do
+        project = create(:empty_project, import_url: generate(:url))
+
         expect(RepositoryImportWorker).to receive(:perform_async).with(project.id)
 
         project.add_import_job
@@ -1735,6 +1745,90 @@ describe Project, models: true do
     end
   end
 
+  describe '#secret_variables_for' do
+    let(:project) { create(:empty_project) }
+
+    let!(:secret_variable) do
+      create(:ci_variable, value: 'secret', project: project)
+    end
+
+    let!(:protected_variable) do
+      create(:ci_variable, :protected, value: 'protected', project: project)
+    end
+
+    subject { project.secret_variables_for('ref') }
+
+    shared_examples 'ref is protected' do
+      it 'contains all the variables' do
+        is_expected.to contain_exactly(secret_variable, protected_variable)
+      end
+    end
+
+    context 'when the ref is not protected' do
+      before do
+        stub_application_setting(
+          default_branch_protection: Gitlab::Access::PROTECTION_NONE)
+      end
+
+      it 'contains only the secret variables' do
+        is_expected.to contain_exactly(secret_variable)
+      end
+    end
+
+    context 'when the ref is a protected branch' do
+      before do
+        create(:protected_branch, name: 'ref', project: project)
+      end
+
+      it_behaves_like 'ref is protected'
+    end
+
+    context 'when the ref is a protected tag' do
+      before do
+        create(:protected_tag, name: 'ref', project: project)
+      end
+
+      it_behaves_like 'ref is protected'
+    end
+  end
+
+  describe '#protected_for?' do
+    let(:project) { create(:empty_project) }
+
+    subject { project.protected_for?('ref') }
+
+    context 'when the ref is not protected' do
+      before do
+        stub_application_setting(
+          default_branch_protection: Gitlab::Access::PROTECTION_NONE)
+      end
+
+      it 'returns false' do
+        is_expected.to be_falsey
+      end
+    end
+
+    context 'when the ref is a protected branch' do
+      before do
+        create(:protected_branch, name: 'ref', project: project)
+      end
+
+      it 'returns true' do
+        is_expected.to be_truthy
+      end
+    end
+
+    context 'when the ref is a protected tag' do
+      before do
+        create(:protected_tag, name: 'ref', project: project)
+      end
+
+      it 'returns true' do
+        is_expected.to be_truthy
+      end
+    end
+  end
+
   describe '#update_project_statistics' do
     let(:project) { create(:empty_project) }
 
@@ -1909,19 +2003,9 @@ describe Project, models: true do
   describe '#http_url_to_repo' do
     let(:project) { create :empty_project }
 
-    context 'when no user is given' do
-      it 'returns the url to the repo without a username' do
-        expect(project.http_url_to_repo).to eq("#{project.web_url}.git")
-        expect(project.http_url_to_repo).not_to include('@')
-      end
-    end
-
-    context 'when user is given' do
-      it 'returns the url to the repo with the username' do
-        user = build_stubbed(:user)
-
-        expect(project.http_url_to_repo(user)).to start_with("http://#{user.username}@")
-      end
+    it 'returns the url to the repo without a username' do
+      expect(project.http_url_to_repo).to eq("#{project.web_url}.git")
+      expect(project.http_url_to_repo).not_to include('@')
     end
   end
 
diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb
index fb2d5f60009..362565506e5 100644
--- a/spec/models/project_team_spec.rb
+++ b/spec/models/project_team_spec.rb
@@ -327,69 +327,114 @@ describe ProjectTeam, models: true do
     end
   end
 
-  shared_examples_for "#max_member_access_for_users" do |enable_request_store|
-    describe "#max_member_access_for_users" do
+  shared_examples 'max member access for users' do
+    let(:project) { create(:project) }
+    let(:group) { create(:group) }
+    let(:second_group) { create(:group) }
+
+    let(:master) { create(:user) }
+    let(:reporter) { create(:user) }
+    let(:guest) { create(:user) }
+
+    let(:promoted_guest) { create(:user) }
+
+    let(:group_developer) { create(:user) }
+    let(:second_developer) { create(:user) }
+
+    let(:user_without_access) { create(:user) }
+    let(:second_user_without_access) { create(:user) }
+
+    let(:users) do
+      [master, reporter, promoted_guest, guest, group_developer, second_developer, user_without_access].map(&:id)
+    end
+
+    let(:expected) do
+      {
+        master.id => Gitlab::Access::MASTER,
+        reporter.id => Gitlab::Access::REPORTER,
+        promoted_guest.id => Gitlab::Access::DEVELOPER,
+        guest.id => Gitlab::Access::GUEST,
+        group_developer.id => Gitlab::Access::DEVELOPER,
+        second_developer.id => Gitlab::Access::MASTER,
+        user_without_access.id => Gitlab::Access::NO_ACCESS
+      }
+    end
+
+    before do
+      project.add_master(master)
+      project.add_reporter(reporter)
+      project.add_guest(promoted_guest)
+      project.add_guest(guest)
+
+      project.project_group_links.create(
+        group: group,
+        group_access: Gitlab::Access::DEVELOPER
+      )
+
+      group.add_master(promoted_guest)
+      group.add_developer(group_developer)
+      group.add_developer(second_developer)
+
+      project.project_group_links.create(
+        group: second_group,
+        group_access: Gitlab::Access::MASTER
+      )
+
+      second_group.add_master(second_developer)
+    end
+
+    it 'returns correct roles for different users' do
+      expect(project.team.max_member_access_for_user_ids(users)).to eq(expected)
+    end
+  end
+
+  describe '#max_member_access_for_user_ids' do
+    context 'with RequestStore enabled' do
       before do
-        RequestStore.begin! if enable_request_store
+        RequestStore.begin!
       end
 
       after do
-        if enable_request_store
-          RequestStore.end!
-          RequestStore.clear!
-        end
+        RequestStore.end!
+        RequestStore.clear!
       end
 
-      it 'returns correct roles for different users' do
-        master = create(:user)
-        reporter = create(:user)
-        promoted_guest = create(:user)
-        guest = create(:user)
-        project = create(:empty_project)
+      include_examples 'max member access for users'
 
-        project.add_master(master)
-        project.add_reporter(reporter)
-        project.add_guest(promoted_guest)
-        project.add_guest(guest)
+      def access_levels(users)
+        project.team.max_member_access_for_user_ids(users)
+      end
+
+      it 'does not perform extra queries when asked for users who have already been found' do
+        access_levels(users)
+
+        expect { access_levels(users) }.not_to exceed_query_limit(0)
 
-        group = create(:group)
-        group_developer = create(:user)
-        second_developer = create(:user)
-        project.project_group_links.create(
-          group: group,
-          group_access: Gitlab::Access::DEVELOPER)
-
-        group.add_master(promoted_guest)
-        group.add_developer(group_developer)
-        group.add_developer(second_developer)
-
-        second_group = create(:group)
-        project.project_group_links.create(
-          group: second_group,
-          group_access: Gitlab::Access::MASTER)
-        second_group.add_master(second_developer)
-
-        users = [master, reporter, promoted_guest, guest, group_developer, second_developer].map(&:id)
-
-        expected = {
-          master.id => Gitlab::Access::MASTER,
-          reporter.id => Gitlab::Access::REPORTER,
-          promoted_guest.id => Gitlab::Access::DEVELOPER,
-          guest.id => Gitlab::Access::GUEST,
-          group_developer.id => Gitlab::Access::DEVELOPER,
-          second_developer.id => Gitlab::Access::MASTER
-        }
-
-        expect(project.team.max_member_access_for_user_ids(users)).to eq(expected)
+        expect(access_levels(users)).to eq(expected)
       end
-    end
-  end
 
-  describe '#max_member_access_for_users with RequestStore' do
-    it_behaves_like "#max_member_access_for_users", true
-  end
+      it 'only requests the extra users when uncached users are passed' do
+        new_user = create(:user)
+        second_new_user = create(:user)
+        all_users = users + [new_user.id, second_new_user.id]
+
+        expected_all = expected.merge(new_user.id => Gitlab::Access::NO_ACCESS,
+                                      second_new_user.id => Gitlab::Access::NO_ACCESS)
+
+        access_levels(users)
 
-  describe '#max_member_access_for_users without RequestStore' do
-    it_behaves_like "#max_member_access_for_users", false
+        queries = ActiveRecord::QueryRecorder.new { access_levels(all_users) }
+
+        expect(queries.count).to eq(1)
+        expect(queries.log_message).to match(/\W#{new_user.id}\W/)
+        expect(queries.log_message).to match(/\W#{second_new_user.id}\W/)
+        expect(queries.log_message).not_to match(/\W#{promoted_guest.id}\W/)
+        expect(access_levels(all_users)).to eq(expected_all)
+      end
+    end
+
+    context 'with RequestStore disabled' do
+      include_examples 'max member access for users'
+    end
   end
 end
diff --git a/spec/models/project_wiki_spec.rb b/spec/models/project_wiki_spec.rb
index 969e9f7a130..224067f58dd 100644
--- a/spec/models/project_wiki_spec.rb
+++ b/spec/models/project_wiki_spec.rb
@@ -37,21 +37,11 @@ describe ProjectWiki, models: true do
   describe "#http_url_to_repo" do
     let(:project) { create :empty_project }
 
-    context 'when no user is given' do
-      it 'returns the url to the repo without a username' do
-        expected_url = "#{Gitlab.config.gitlab.url}/#{subject.path_with_namespace}.git"
+    it 'returns the full http url to the repo' do
+      expected_url = "#{Gitlab.config.gitlab.url}/#{subject.path_with_namespace}.git"
 
-        expect(project_wiki.http_url_to_repo).to eq(expected_url)
-        expect(project_wiki.http_url_to_repo).not_to include('@')
-      end
-    end
-
-    context 'when user is given' do
-      it 'returns the url to the repo with the username' do
-        user = build_stubbed(:user)
-
-        expect(project_wiki.http_url_to_repo(user)).to start_with("http://#{user.username}@")
-      end
+      expect(project_wiki.http_url_to_repo).to eq(expected_url)
+      expect(project_wiki.http_url_to_repo).not_to include('@')
     end
   end
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 9edf34b05ad..a83726b48a0 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -13,6 +13,10 @@ describe User, models: true do
     it { is_expected.to include_module(TokenAuthenticatable) }
   end
 
+  describe 'delegations' do
+    it { is_expected.to delegate_method(:path).to(:namespace).with_prefix }
+  end
+
   describe 'associations' do
     it { is_expected.to have_one(:namespace) }
     it { is_expected.to have_many(:snippets).dependent(:destroy) }
@@ -22,7 +26,7 @@ describe User, models: true do
     it { is_expected.to have_many(:deploy_keys).dependent(:destroy) }
     it { is_expected.to have_many(:events).dependent(:destroy) }
     it { is_expected.to have_many(:recent_events).class_name('Event') }
-    it { is_expected.to have_many(:issues).dependent(:restrict_with_exception) }
+    it { is_expected.to have_many(:issues).dependent(:destroy) }
     it { is_expected.to have_many(:notes).dependent(:destroy) }
     it { is_expected.to have_many(:merge_requests).dependent(:destroy) }
     it { is_expected.to have_many(:identities).dependent(:destroy) }
@@ -1496,25 +1500,6 @@ describe User, models: true do
     end
   end
 
-  describe '#viewable_starred_projects' do
-    let(:user) { create(:user) }
-    let(:public_project) { create(:empty_project, :public) }
-    let(:private_project) { create(:empty_project, :private) }
-    let(:private_viewable_project) { create(:empty_project, :private) }
-
-    before do
-      private_viewable_project.team << [user, Gitlab::Access::MASTER]
-
-      [public_project, private_project, private_viewable_project].each do |project|
-        user.toggle_star(project)
-      end
-    end
-
-    it 'returns only starred projects the user can view' do
-      expect(user.viewable_starred_projects).not_to include(private_project)
-    end
-  end
-
   describe '#projects_with_reporter_access_limited_to' do
     let(:project1) { create(:empty_project) }
     let(:project2) { create(:empty_project) }