Merge branch 'limit-number-of-shown-environments' into 'master'

Limit number of shown environments

## What does this MR do?

This MR limits in context of Merge Request a list of shown environments. Previously we would show all environments containing the SHA of the head commit of Merge Request. However, with introducing of dynamically created environments this lead to a cases that we would show multiple review apps, for different branches, because these branches would contain a new questioned commit.

This MR changes what environments we test against presence of the commit, to:
1. We look for environments with deployments to source_branch of source_project: used for deployments to per-branch environments,
2. We look for environments with deployments to target_branch of target_project: used for deployments to staging / production environments,
3. We look for environments with deployments for tags on target_project: used for staging / production environments.

## Why was this MR needed?

To improve a list of returned environments when we introduced ability to create dynamic environments for review apps:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6323

See merge request !6438

CHANGELOG

@@ -24,6 +24,7 @@ v 8.12.0 (unreleased)
   - Cycle analytics (first iteration) !5986
   - Remove vendor prefixes for linear-gradient CSS (ClemMakesApps)
   - Move pushes_since_gc from the database to Redis
+  - Limit number of shown environments on Merge Request: show only environments for target_branch, source_branch and tags
   - Add font color contrast to external label in admin area (ClemMakesApps)
   - Change logo animation to CSS (ClemMakesApps)
   - Instructions for enabling Git packfile bitmaps !6104

app/helpers/gitlab_routing_helper.rb

@@ -70,6 +70,10 @@ module GitlabRoutingHelper
     namespace_project_runner_path(@project.namespace, @project, runner, *args)
   end
 
+  def environment_path(environment, *args)
+    namespace_project_environment_path(environment.project.namespace, environment.project, environment, *args)
+  end
+
   def issue_path(entity, *args)
     namespace_project_issue_path(entity.project.namespace, entity.project, entity, *args)
   end

app/models/merge_request.rb

@@ -670,9 +670,12 @@ class MergeRequest < ActiveRecord::Base
   def environments
     return [] unless diff_head_commit
 
-    target_project.environments.select do |environment|
+    environments = source_project.environments_for(
-      environment.includes_commit?(diff_head_commit)
+      source_branch, diff_head_commit)
-    end
+    environments += target_project.environments_for(
+      target_branch, diff_head_commit, with_tags: true)
+
+    environments.uniq
   end
 
   def state_human_name

app/models/project.rb

@@ -1293,6 +1293,22 @@ class Project < ActiveRecord::Base
     Gitlab::Redis.with { |redis| redis.del(pushes_since_gc_redis_key) }
   end
 
+  def environments_for(ref, commit, with_tags: false)
+    environment_ids = deployments.group(:environment_id).
+      select(:environment_id)
+
+    environment_ids =
+      if with_tags
+        environment_ids.where('ref=? OR tag IS TRUE', ref)
+      else
+        environment_ids.where(ref: ref)
+      end
+
+    environments.where(id: environment_ids).select do |environment|
+      environment.includes_commit?(commit)
+    end
+  end
+
   private
 
   def pushes_since_gc_redis_key

app/views/projects/merge_requests/widget/_heading.html.haml

@@ -43,14 +43,15 @@
       = icon("times-circle")
       Could not connect to the CI server. Please check your settings and try again.
 
-- @merge_request.environments.each do |environment|
+- @merge_request.environments.sort_by(&:name).each do |environment|
+  - if can?(current_user, :read_environment, environment)
     .mr-widget-heading
       .ci_widget.ci-success
         = ci_icon_for_status("success")
         %span.hidden-sm
           Deployed to
           = succeed '.' do
-          = link_to environment.name, namespace_project_environment_path(@project.namespace, @project, environment), class: 'environment'
+            = link_to environment.name, environment_path(environment), class: 'environment'
           - external_url = environment.external_url
           - if external_url
             = link_to external_url, target: '_blank' do

spec/models/merge_request_spec.rb

@@ -701,16 +701,57 @@ describe MergeRequest, models: true do
     end
   end
 
-  describe "#environments" do
+  describe '#environments' do
     let(:project)       { create(:project) }
     let(:merge_request) { create(:merge_request, source_project: project) }
 
-    it 'selects deployed environments' do
+    context 'with multiple environments' do
-      environments = create_list(:environment, 3, project: project)
+      let(:environments) { create_list(:environment, 3, project: project) }
-      create(:deployment, environment: environments.first, sha: project.commit('master').id)
-      create(:deployment, environment: environments.second, sha: project.commit('feature').id)
 
-      expect(merge_request.environments).to eq [environments.first]
+      before do
+        create(:deployment, environment: environments.first, ref: 'master', sha: project.commit('master').id)
+        create(:deployment, environment: environments.second, ref: 'feature', sha: project.commit('feature').id)
+      end
+
+      it 'selects deployed environments' do
+        expect(merge_request.environments).to contain_exactly(environments.first)
+      end
+    end
+
+    context 'with environments on source project' do
+      let(:source_project) do
+        create(:project) do |fork_project|
+          fork_project.create_forked_project_link(forked_to_project_id: fork_project.id, forked_from_project_id: project.id)
+        end
+      end
+
+      let(:merge_request) do
+        create(:merge_request,
+               source_project: source_project, source_branch: 'feature',
+               target_project: project)
+      end
+
+      let(:source_environment) { create(:environment, project: source_project) }
+
+      before do
+        create(:deployment, environment: source_environment, ref: 'feature', sha: merge_request.diff_head_sha)
+      end
+
+      it 'selects deployed environments' do
+        expect(merge_request.environments).to contain_exactly(source_environment)
+      end
+
+      context 'with environments on target project' do
+        let(:target_environment) { create(:environment, project: project) }
+
+        before do
+          create(:deployment, environment: target_environment, tag: true, sha: merge_request.diff_head_sha)
+        end
+
+        it 'selects deployed environments' do
+          expect(merge_request.environments).to contain_exactly(source_environment, target_environment)
+        end
+      end
     end
 
     context 'without a diff_head_commit' do

spec/models/project_spec.rb

@@ -1647,6 +1647,47 @@ describe Project, models: true do
     end
   end
 
+  describe '#environments_for' do
+    let(:project) { create(:project) }
+    let(:environment) { create(:environment, project: project) }
+
+    context 'tagged deployment' do
+      before do
+        create(:deployment, environment: environment, ref: '1.0', tag: true, sha: project.commit.id)
+      end
+
+      it 'returns environment when with_tags is set' do
+        expect(project.environments_for('master', 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', project.commit)).to be_empty
+      end
+
+      it 'does not return environment when commit is not part of deployment' do
+        expect(project.environments_for('master', project.commit('feature'))).to be_empty
+      end
+    end
+
+    context 'branch deployment' do
+      before do
+        create(:deployment, environment: environment, ref: 'master', sha: project.commit.id)
+      end
+
+      it 'returns environment when ref is set' do
+        expect(project.environments_for('master', project.commit)).to contain_exactly(environment)
+      end
+
+      it 'does not environment when ref is different' do
+        expect(project.environments_for('feature', project.commit)).to be_empty
+      end
+
+      it 'does not return environment when commit is not part of deployment' do
+        expect(project.environments_for('master', project.commit('feature'))).to be_empty
+      end
+    end
+  end
+
   def enable_lfs
     allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
   end

spec/views/projects/merge_requests/_heading.html.haml_spec.rb

@@ -15,6 +15,8 @@ describe 'projects/merge_requests/widget/_heading' do
       assign(:merge_request, merge_request)
       assign(:project, project)
 
+      allow(view).to receive(:can?).and_return(true)
+
       render
     end