Merge branch 'fix-environment-status-in-merge-request-widget' into 'master'

Fix environment status in merge request widget

Closes #51120 and #25140

See merge request gitlab-org/gitlab-ce!22799

app/models/ci/build.rb

@@ -211,6 +211,7 @@ module Ci
         build.deployment&.succeed
 
         build.run_after_commit do
+          BuildSuccessWorker.perform_async(id)
           PagesWorker.perform_async(:deploy, id) if build.pages_generator?
         end
       end

app/models/deployment.rb

@@ -10,7 +10,9 @@ class Deployment < ActiveRecord::Base
   belongs_to :user
   belongs_to :deployable, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
 
-  has_internal_id :iid, scope: :project, init: ->(s) { s&.project&.deployments&.maximum(:iid) }
+  has_internal_id :iid, scope: :project, init: ->(s) do
+    Deployment.where(project: s.project).maximum(:iid) if s&.project
+  end
 
   validates :sha, presence: true
   validates :ref, presence: true

app/models/environment.rb

@@ -50,6 +50,7 @@ class Environment < ActiveRecord::Base
   scope :in_review_folder, -> { where(environment_type: "review") }
   scope :for_name, -> (name) { where(name: name) }
   scope :for_project, -> (project) { where(project_id: project) }
+  scope :with_deployment, -> (sha) { where('EXISTS (?)', Deployment.select(1).where('deployments.environment_id = environments.id').where(sha: sha)) }
 
   state_machine :state, initial: :available do
     event :start do

app/models/environment_status.rb

@@ -8,17 +8,16 @@ class EnvironmentStatus
   delegate :id, to: :environment
   delegate :name, to: :environment
   delegate :project, to: :environment
-  delegate :status, to: :deployment, allow_nil: true
   delegate :deployed_at, to: :deployment, allow_nil: true
 
   def self.for_merge_request(mr, user)
-    build_environments_status(mr, user, mr.head_pipeline)
+    build_environments_status(mr, user, mr.diff_head_sha)
   end
 
   def self.after_merge_request(mr, user)
     return [] unless mr.merged?
 
-    build_environments_status(mr, user, mr.merge_pipeline)
+    build_environments_status(mr, user, mr.merge_commit_sha)
   end
 
   def initialize(environment, merge_request, sha)
@@ -29,7 +28,7 @@ class EnvironmentStatus
 
   def deployment
     strong_memoize(:deployment) do
-      environment.first_deployment_for(sha)
+      Deployment.where(environment: environment).find_by_sha(sha)
     end
   end
 
@@ -44,6 +43,22 @@ class EnvironmentStatus
       .merge_request_diff_files.where(deleted_file: false)
   end
 
+  ##
+  # Since frontend has not supported all statuses yet, BE has to
+  # proxy some status to a supported status.
+  def status
+    return unless deployment
+
+    case deployment.status
+    when 'created'
+      'running'
+    when 'canceled'
+      'failed'
+    else
+      deployment.status
+    end
+  end
+
   private
 
   PAGE_EXTENSIONS = /\A\.(s?html?|php|asp|cgi|pl)\z/i.freeze
@@ -61,21 +76,14 @@ class EnvironmentStatus
     }
   end
 
-  def self.build_environments_status(mr, user, pipeline)
-    return [] unless pipeline.present?
+  def self.build_environments_status(mr, user, sha)
+    Environment.where(project_id: [mr.source_project_id, mr.target_project_id])
+               .available
+               .with_deployment(sha).map do |environment|
+      next unless Ability.allowed?(user, :read_environment, environment)
 
-    find_environments(user, pipeline).map do |environment|
-      EnvironmentStatus.new(environment, mr, pipeline.sha)
-    end
+      EnvironmentStatus.new(environment, mr, sha)
+    end.compact
   end
   private_class_method :build_environments_status
-
-  def self.find_environments(user, pipeline)
-    env_ids = Deployment.where(deployable: pipeline.builds).select(:environment_id)
-
-    Environment.available.where(id: env_ids).select do |environment|
-      Ability.allowed?(user, :read_environment, environment)
-    end
-  end
-  private_class_method :find_environments
 end

changelogs/unreleased/add-action-to-deployment.yml

+---
+title: Fix environment status in merge request widget
+merge_request: 22799
+author:
+type: changed

db/migrate/20181106135939_add_index_to_deployments.rb

+# frozen_string_literal: true
+
+class AddIndexToDeployments < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :deployments, [:project_id, :status, :created_at]
+  end
+
+  def down
+    remove_concurrent_index :deployments, [:project_id, :status, :created_at]
+  end
+end

db/schema.rb

@@ -837,6 +837,7 @@ ActiveRecord::Schema.define(version: 20181107054254) do
   add_index "deployments", ["environment_id", "status"], name: "index_deployments_on_environment_id_and_status", using: :btree
   add_index "deployments", ["id"], name: "partial_index_deployments_for_legacy_successful_deployments", where: "((finished_at IS NULL) AND (status = 2))", using: :btree
   add_index "deployments", ["project_id", "iid"], name: "index_deployments_on_project_id_and_iid", unique: true, using: :btree
+  add_index "deployments", ["project_id", "status", "created_at"], name: "index_deployments_on_project_id_and_status_and_created_at", using: :btree
   add_index "deployments", ["project_id", "status"], name: "index_deployments_on_project_id_and_status", using: :btree
 
   create_table "emails", force: :cascade do |t|

spec/features/merge_request/user_sees_deployment_widget_spec.rb

 require 'rails_helper'
 
 describe 'Merge request > User sees deployment widget', :js do
-  describe 'when deployed to an environment' do
+  describe 'when merge request has associated environments' do
     let(:user) { create(:user) }
     let(:project) { create(:project, :repository) }
     let(:merge_request) { create(:merge_request, :merged, source_project: project) }
@@ -10,30 +10,74 @@ describe 'Merge request > User sees deployment widget', :js do
     let(:ref) { merge_request.target_branch }
     let(:sha) { project.commit(ref).id }
     let(:pipeline) { create(:ci_pipeline_without_jobs, sha: sha, project: project, ref: ref) }
-    let(:build)    { create(:ci_build, :success, pipeline: pipeline) }
-    let!(:deployment) { create(:deployment, :succeed, environment: environment, sha: sha, ref: ref, deployable: build) }
     let!(:manual) { }
 
     before do
       merge_request.update!(merge_commit_sha: sha)
       project.add_user(user, role)
       sign_in(user)
-      visit project_merge_request_path(project, merge_request)
-      wait_for_requests
     end
 
-    it 'displays that the environment is deployed' do
-      wait_for_requests
+    context 'when deployment succeeded' do
+      let(:build) { create(:ci_build, :success, pipeline: pipeline) }
+      let!(:deployment) { create(:deployment, :succeed, environment: environment, sha: sha, ref: ref, deployable: build) }
 
-      expect(page).to have_content("Deployed to #{environment.name}")
-      expect(find('.js-deploy-time')['data-original-title']).to eq(deployment.created_at.to_time.in_time_zone.to_s(:medium))
+      it 'displays that the environment is deployed' do
+        visit project_merge_request_path(project, merge_request)
+        wait_for_requests
+
+        expect(page).to have_content("Deployed to #{environment.name}")
+        expect(find('.js-deploy-time')['data-original-title']).to eq(deployment.created_at.to_time.in_time_zone.to_s(:medium))
+      end
+    end
+
+    context 'when deployment failed' do
+      let(:build) { create(:ci_build, :failed, pipeline: pipeline) }
+      let!(:deployment) { create(:deployment, :failed, environment: environment, sha: sha, ref: ref, deployable: build) }
+
+      it 'displays that the deployment failed' do
+        visit project_merge_request_path(project, merge_request)
+        wait_for_requests
+
+        expect(page).to have_content("Failed to deploy to #{environment.name}")
+        expect(page).not_to have_css('.js-deploy-time')
+      end
+    end
+
+    context 'when deployment running' do
+      let(:build) { create(:ci_build, :running, pipeline: pipeline) }
+      let!(:deployment) { create(:deployment, :running, environment: environment, sha: sha, ref: ref, deployable: build) }
+
+      it 'displays that the running deployment' do
+        visit project_merge_request_path(project, merge_request)
+        wait_for_requests
+
+        expect(page).to have_content("Deploying to #{environment.name}")
+        expect(page).not_to have_css('.js-deploy-time')
+      end
+    end
+
+    context 'when deployment will happen' do
+      let(:build) { create(:ci_build, :created, pipeline: pipeline) }
+      let!(:deployment) { create(:deployment, environment: environment, sha: sha, ref: ref, deployable: build) }
+
+      it 'displays that the environment name' do
+        visit project_merge_request_path(project, merge_request)
+        wait_for_requests
+
+        expect(page).to have_content("Deploying to #{environment.name}")
+        expect(page).not_to have_css('.js-deploy-time')
+      end
     end
 
     context 'with stop action' do
+      let(:build) { create(:ci_build, :success, pipeline: pipeline) }
+      let!(:deployment) { create(:deployment, :succeed, environment: environment, sha: sha, ref: ref, deployable: build) }
       let(:manual) { create(:ci_build, :manual, pipeline: pipeline, name: 'close_app') }
 
       before do
         deployment.update!(on_stop: manual.name)
+        visit project_merge_request_path(project, merge_request)
         wait_for_requests
       end
 

spec/features/projects/environments/environment_spec.rb

@@ -25,7 +25,7 @@ describe 'Environment' do
     end
 
     context 'without deployments' do
-      it 'does show no deployments' do
+      it 'does not show deployments' do
         expect(page).to have_content('You don\'t have any deployments right now.')
       end
     end
@@ -43,6 +43,45 @@ describe 'Environment' do
         end
       end
 
+      context 'when there is a successful deployment' do
+        let(:pipeline) { create(:ci_pipeline, project: project) }
+        let(:build) { create(:ci_build, :success, pipeline: pipeline) }
+
+        let(:deployment) do
+          create(:deployment, :success, environment: environment, deployable: build)
+        end
+
+        it 'does show deployments' do
+          expect(page).to have_link("#{build.name} (##{build.id})")
+        end
+      end
+
+      context 'when there is a running deployment' do
+        let(:pipeline) { create(:ci_pipeline, project: project) }
+        let(:build) { create(:ci_build, pipeline: pipeline) }
+
+        let(:deployment) do
+          create(:deployment, :running, environment: environment, deployable: build)
+        end
+
+        it 'does not show deployments' do
+          expect(page).to have_content('You don\'t have any deployments right now.')
+        end
+      end
+
+      context 'when there is a failed deployment' do
+        let(:pipeline) { create(:ci_pipeline, project: project) }
+        let(:build) { create(:ci_build, pipeline: pipeline) }
+
+        let(:deployment) do
+          create(:deployment, :failed, environment: environment, deployable: build)
+        end
+
+        it 'does not show deployments' do
+          expect(page).to have_content('You don\'t have any deployments right now.')
+        end
+      end
+
       context 'with related deployable present' do
         let(:pipeline) { create(:ci_pipeline, project: project) }
         let(:build) { create(:ci_build, pipeline: pipeline) }

spec/features/projects/environments/environments_spec.rb

@@ -128,7 +128,7 @@ describe 'Environments page', :js do
       end
     end
 
-    context 'when there are deployments' do
+    context 'when there are successful deployments' do
       let(:project) { create(:project, :repository) }
 
       let!(:deployment) do
@@ -328,6 +328,22 @@ describe 'Environments page', :js do
         end
       end
     end
+
+    context 'when there is a failed deployment' do
+      let(:project) { create(:project, :repository) }
+
+      let!(:deployment) do
+        create(:deployment, :failed,
+                            environment: environment,
+                            sha: project.commit.id)
+      end
+
+      it 'does not show deployments' do
+        visit_environments(project)
+
+        expect(page).to have_content('No deployments yet')
+      end
+    end
   end
 
   it 'does have a new environment button' do

spec/models/environment_spec.rb

@@ -53,6 +53,25 @@ describe Environment do
     end
   end
 
+  describe '.with_deployment' do
+    subject { described_class.with_deployment(sha) }
+
+    let(:environment) { create(:environment) }
+    let(:sha) { RepoHelpers.sample_commit.id }
+
+    context 'when deployment has the specified sha' do
+      let!(:deployment) { create(:deployment, environment: environment, sha: sha) }
+
+      it { is_expected.to eq([environment]) }
+    end
+
+    context 'when deployment does not have the specified sha' do
+      let!(:deployment) { create(:deployment, environment: environment, sha: 'abc') }
+
+      it { is_expected.to be_empty }
+    end
+  end
+
   describe '#folder_name' do
     context 'when it is inside a folder' do
       subject(:environment) do

spec/models/environment_status_spec.rb

 require 'spec_helper'
 
 describe EnvironmentStatus do
+  include ProjectForksHelper
+
   let(:deployment)    { create(:deployment, :succeed, :review_app) }
-  let(:environment)   { deployment.environment}
+  let(:environment)   { deployment.environment }
   let(:project)       { deployment.project }
   let(:merge_request) { create(:merge_request, :deployed_review_app, deployment: deployment) }
   let(:sha)           { deployment.sha }
@@ -65,9 +67,9 @@ describe EnvironmentStatus do
     let(:admin)    { create(:admin) }
     let(:pipeline) { create(:ci_pipeline, sha: sha) }
 
-    it 'is based on merge_request.head_pipeline' do
-      expect(merge_request).to receive(:head_pipeline).and_return(pipeline)
-      expect(merge_request).not_to receive(:merge_pipeline)
+    it 'is based on merge_request.diff_head_sha' do
+      expect(merge_request).to receive(:diff_head_sha)
+      expect(merge_request).not_to receive(:merge_commit_sha)
 
       described_class.for_merge_request(merge_request, admin)
     end
@@ -81,11 +83,83 @@ describe EnvironmentStatus do
       merge_request.mark_as_merged!
     end
 
-    it 'is based on merge_request.merge_pipeline' do
-      expect(merge_request).to receive(:merge_pipeline).and_return(pipeline)
-      expect(merge_request).not_to receive(:head_pipeline)
+    it 'is based on merge_request.merge_commit_sha' do
+      expect(merge_request).to receive(:merge_commit_sha)
+      expect(merge_request).not_to receive(:diff_head_sha)
 
       described_class.after_merge_request(merge_request, admin)
     end
   end
+
+  describe '.build_environments_status' do
+    subject { described_class.send(:build_environments_status, merge_request, user, sha) }
+
+    let!(:build) { create(:ci_build, :deploy_to_production, pipeline: pipeline) }
+    let(:environment) { build.deployment.environment }
+    let(:user) { project.owner }
+
+    before do
+      build.deployment&.update!(sha: sha)
+    end
+
+    context 'when environment is created on a forked project' do
+      let(:project) { create(:project, :repository) }
+      let(:forked) { fork_project(project, user, repository: true) }
+      let(:sha) { forked.commit.sha }
+      let(:pipeline) { create(:ci_pipeline, sha: sha, project: forked) }
+
+      let(:merge_request) do
+        create(:merge_request,
+               source_project: forked,
+               target_project: project,
+               target_branch: 'master',
+               head_pipeline: pipeline)
+      end
+
+      it 'returns environment status' do
+        expect(subject.count).to eq(1)
+        expect(subject[0].environment).to eq(environment)
+        expect(subject[0].merge_request).to eq(merge_request)
+        expect(subject[0].sha).to eq(sha)
+      end
+    end
+
+    context 'when environment is created on a target project' do
+      let(:project) { create(:project, :repository) }
+      let(:sha) { project.commit.sha }
+      let(:pipeline) { create(:ci_pipeline, sha: sha, project: project) }
+
+      let(:merge_request) do
+        create(:merge_request,
+               source_project: project,
+               source_branch: 'feature',
+               target_project: project,
+               target_branch: 'master',
+               head_pipeline: pipeline)
+      end
+
+      it 'returns environment status' do
+        expect(subject.count).to eq(1)
+        expect(subject[0].environment).to eq(environment)
+        expect(subject[0].merge_request).to eq(merge_request)
+        expect(subject[0].sha).to eq(sha)
+      end
+
+      context 'when the build stops an environment' do
+        let!(:build) { create(:ci_build, :stop_review_app, pipeline: pipeline) }
+
+        it 'does not return environment status' do
+          expect(subject.count).to eq(0)
+        end
+      end
+
+      context 'when user does not have a permission to see the environment' do
+        let(:user) { create(:user) }
+
+        it 'does not return environment status' do
+          expect(subject.count).to eq(0)
+        end
+      end
+    end
+  end
 end

spec/serializers/environment_status_entity_spec.rb

@@ -15,6 +15,7 @@ describe EnvironmentStatusEntity do
   subject { entity.as_json }
 
   before do
+    deployment.update(sha: merge_request.diff_head_sha)
     allow(request).to receive(:current_user).and_return(user)
   end