Improve performance of finding last deployed environment

4 files changed, 30 insertions, 79 deletions

diff --git a/spec/lib/gitlab/database_spec.rb b/spec/lib/gitlab/database_spec.rb
index 3031559c613..b142b3a2781 100644
--- a/spec/lib/gitlab/database_spec.rb
+++ b/spec/lib/gitlab/database_spec.rb
@@ -55,6 +55,22 @@ describe Gitlab::Database, lib: true do
     end
   end
 
+  describe '.nulls_first_order' do
+    context 'when using PostgreSQL' do
+      before { expect(described_class).to receive(:postgresql?).and_return(true) }
+
+      it { expect(described_class.nulls_first_order('column', 'ASC')).to eq 'column ASC NULLS FIRST'}
+      it { expect(described_class.nulls_first_order('column', 'DESC')).to eq 'column DESC NULLS FIRST'}
+    end
+
+    context 'when using MySQL' do
+      before { expect(described_class).to receive(:postgresql?).and_return(false) }
+
+      it { expect(described_class.nulls_first_order('column', 'ASC')).to eq 'column ASC'}
+      it { expect(described_class.nulls_first_order('column', 'DESC')).to eq 'column IS NULL, column DESC'}
+    end
+  end
+
   describe '#true_value' do
     it 'returns correct value for PostgreSQL' do
       expect(described_class).to receive(:postgresql?).and_return(true)
diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb
index e40a9bf4fbe..7b68d0756d0 100644
--- a/spec/models/environment_spec.rb
+++ b/spec/models/environment_spec.rb
@@ -22,22 +22,17 @@ describe Environment, models: true do
   it { is_expected.to validate_length_of(:external_url).is_at_most(255) }
   it { is_expected.to validate_uniqueness_of(:external_url).scoped_to(:project_id) }
 
-  describe '.latest_for_commit' do
+  describe '.order_by_last_deployed_at' do
+    let(:project) { create(:project) }
     let!(:environment1) { create(:environment, project: project) }
     let!(:environment2) { create(:environment, project: project) }
     let!(:environment3) { create(:environment, project: project) }
-    let!(:deployment1) { create(:deployment, environment: environment1) }
     let!(:deployment2) { create(:deployment, environment: environment2) }
-    let(:commit) { RepoHelpers.sample_commit }
-
-    before do
-      allow(environment1).to receive(:first_deployment_for).with(commit).and_return(deployment1)
-      allow(environment2).to receive(:first_deployment_for).with(commit).and_return(deployment2)
-      allow(environment3).to receive(:first_deployment_for).with(commit).and_return(nil)
-    end
+    let!(:deployment1) { create(:deployment, environment: environment1) }
 
-    it 'returns the environment that the commit was last deployed to' do
-      expect(Environment.latest_for_commit([environment1, environment2, environment3], commit)).to be(environment2)
+    it 'returns the environments in order of having been last deployed' do
+      # byebug
+      expect(project.environments.order_by_last_deployed_at.to_a).to eq([environment3, environment2, environment1])
     end
   end
 
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 91a8f2d77ab..32ed1e96749 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1069,30 +1069,6 @@ describe MergeRequest, models: true do
     end
   end
 
-  describe '#latest_environment' do
-    let(:project) { subject.project }
-    let!(:environment1) { create(:environment, project: project) }
-    let!(:environment2) { create(:environment, project: project) }
-    let!(:environment3) { create(:environment, project: project) }
-    let!(:deployment1) { create(:deployment, environment: environment1, ref: 'master', sha: commit.id) }
-    let!(:deployment2) { create(:deployment, environment: environment2, ref: 'feature', sha: commit.id) }
-    let(:commit) { subject.diff_head_commit }
-
-    before do
-      allow(environment1).to receive(:first_deployment_for).with(commit).and_return(deployment1)
-      allow(environment2).to receive(:first_deployment_for).with(commit).and_return(deployment2)
-      allow(environment3).to receive(:first_deployment_for).with(commit).and_return(nil)
-    end
-
-    before do
-      allow(subject).to receive(:environments).and_return([environment1, environment2, environment3])
-    end
-
-    it 'returns the environment that the commit was last deployed to' do
-      expect(subject.latest_environment).to eq(environment2)
-    end
-  end
-
   describe "#reload_diff" do
     let(:note) { create(:diff_note_on_merge_request, project: subject.project, noteable: subject) }
 
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 1072b324b22..2280d0f554a 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1726,17 +1726,17 @@ describe Project, models: true do
       end
 
       it 'returns environment when with_tags is set' do
-        expect(project.environments_for('master', commit: project.commit, with_tags: true))
+        expect(project.environments_for(ref: 'master', commit: project.commit, with_tags: true))
           .to contain_exactly(environment)
       end
 
       it 'does not return environment when no with_tags is set' do
-        expect(project.environments_for('master', commit: project.commit))
+        expect(project.environments_for(ref: 'master', commit: project.commit))
           .to be_empty
       end
 
       it 'does not return environment when commit is not part of deployment' do
-        expect(project.environments_for('master', commit: project.commit('feature')))
+        expect(project.environments_for(ref: 'master', commit: project.commit('feature')))
           .to be_empty
       end
     end
@@ -1747,22 +1747,22 @@ describe Project, models: true do
       end
 
       it 'returns environment when ref is set' do
-        expect(project.environments_for('master', commit: project.commit))
+        expect(project.environments_for(ref: 'master', commit: project.commit))
           .to contain_exactly(environment)
       end
 
       it 'does not environment when ref is different' do
-        expect(project.environments_for('feature', commit: project.commit))
+        expect(project.environments_for(ref: 'feature', commit: project.commit))
           .to be_empty
       end
 
       it 'does not return environment when commit is not part of deployment' do
-        expect(project.environments_for('master', commit: project.commit('feature')))
+        expect(project.environments_for(ref: 'master', commit: project.commit('feature')))
           .to be_empty
       end
 
       it 'returns environment when commit constraint is not set' do
-        expect(project.environments_for('master'))
+        expect(project.environments_for(ref: 'master'))
           .to contain_exactly(environment)
       end
     end
@@ -1773,48 +1773,12 @@ describe Project, models: true do
       end
 
       it 'returns environment' do
-        expect(project.environments_for(nil, commit: project.commit))
+        expect(project.environments_for(commit: project.commit))
           .to contain_exactly(environment)
       end
     end
   end
 
-  describe '#latest_environment_for' do
-    let(:project) { create(:project) }
-    let!(:environment1) { create(:environment, project: project) }
-    let!(:environment2) { create(:environment, project: project) }
-    let!(:environment3) { create(:environment, project: project) }
-    let!(:deployment1) { create(:deployment, environment: environment1, ref: 'master', sha: commit.id) }
-    let!(:deployment2) { create(:deployment, environment: environment2, ref: 'feature', sha: commit.id) }
-    let(:commit) { project.commit }
-
-    before do
-      allow(environment1).to receive(:first_deployment_for).with(commit).and_return(deployment1)
-      allow(environment2).to receive(:first_deployment_for).with(commit).and_return(deployment2)
-      allow(environment3).to receive(:first_deployment_for).with(commit).and_return(nil)
-    end
-
-    context 'when specifying a ref' do
-      before do
-        allow(project).to receive(:environments_for).with('master', commit: commit).and_return([environment1])
-      end
-
-      it 'returns the environment that the commit was last deployed to from that ref' do
-        expect(project.latest_environment_for(commit, ref: 'master')).to eq(environment1)
-      end
-    end
-
-    context 'when not specifying a ref' do
-      before do
-        allow(project).to receive(:environments_for).with(nil, commit: commit).and_return([environment1, environment2])
-      end
-
-      it 'returns the environment that the commit was last deployed to' do
-        expect(project.latest_environment_for(commit)).to eq(environment2)
-      end
-    end
-  end
-
   describe '#environments_recently_updated_on_branch' do
     let(:project) { create(:project, :repository) }
     let(:environment) { create(:environment, project: project) }