Merge branch 'fix-mr-widget-unrelated-deployment-status' into 'master'

Fix unrelated deployment status in MR widget

Closes #54125

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

app/models/ci/pipeline.rb

@@ -26,6 +26,8 @@ class Pipeline < ActiveRecord::Base
     has_many :builds, foreign_key: :commit_id, inverse_of: :pipeline
     has_many :trigger_requests, dependent: :destroy, foreign_key: :commit_id # rubocop:disable Cop/ActiveRecordDependent
     has_many :variables, class_name: 'Ci::PipelineVariable'
+    has_many :deployments, through: :builds
+    has_many :environments, -> { distinct }, through: :deployments
 
     # Merge requests for which the current pipeline is running against
     # the merge request's latest commit.
@@ -523,10 +525,6 @@ def has_yaml_errors?
       yaml_errors.present?
     end
 
-    def environments
-      builds.where.not(environment: nil).success.pluck(:environment).uniq
-    end
-
     # Manually set the notes for a Ci::Pipeline
     # There is no ActiveRecord relation between Ci::Pipeline and notes
     # as they are related to a commit sha. This method helps importing

app/models/environment_status.rb

@@ -12,13 +12,13 @@ class EnvironmentStatus
   delegate :deployed_at, to: :deployment, allow_nil: true
 
   def self.for_merge_request(mr, user)
-    build_environments_status(mr, user, mr.diff_head_sha)
+    build_environments_status(mr, user, mr.actual_head_pipeline)
   end
 
   def self.after_merge_request(mr, user)
     return [] unless mr.merged?
 
-    build_environments_status(mr, user, mr.merge_commit_sha)
+    build_environments_status(mr, user, mr.merge_pipeline)
   end
 
   def initialize(environment, merge_request, sha)
@@ -61,13 +61,13 @@ def build_change(file)
     }
   end
 
-  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|
+  def self.build_environments_status(mr, user, pipeline)
+    return [] unless pipeline
+
+    pipeline.environments.available.map do |environment|
       next unless Ability.allowed?(user, :read_environment, environment)
 
-      EnvironmentStatus.new(environment, mr, sha)
+      EnvironmentStatus.new(environment, mr, pipeline.sha)
     end.compact
   end
   private_class_method :build_environments_status

changelogs/unreleased/fix-mr-widget-unrelated-deployment-status.yml

@@ -0,0 +1,5 @@
+---
+title: Fix unrelated deployment status in MR widget
+merge_request: 23175
+author:
+type: fixed

spec/features/merge_request/user_sees_deployment_widget_spec.rb

@@ -29,6 +29,22 @@
         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
+
+      context 'when a user created a new merge request with the same SHA' do
+        let(:pipeline2) { create(:ci_pipeline_without_jobs, sha: sha, project: project, ref: 'new-patch-1') }
+        let(:build2) { create(:ci_build, :success, pipeline: pipeline2) }
+        let(:environment2) { create(:environment, project: project) }
+        let!(:deployment2) { create(:deployment, environment: environment2, sha: sha, ref: 'new-patch-1', deployable: build2) }
+
+        it 'displays one environment which is related to the pipeline' do
+          visit project_merge_request_path(project, merge_request)
+          wait_for_requests
+
+          expect(page).to have_selector('.js-deployment-info', count: 1)
+          expect(page).to have_content("#{environment.name}")
+          expect(page).not_to have_content("#{environment2.name}")
+        end
+      end
     end
 
     context 'when deployment failed' do

spec/lib/gitlab/import_export/all_models.yml

@@ -121,6 +121,8 @@ pipelines:
 - artifacts
 - pipeline_schedule
 - merge_requests
+- deployments
+- environments
 pipeline_variables:
 - pipeline
 stages:

spec/models/environment_status_spec.rb

@@ -92,16 +92,12 @@
   end
 
   describe '.build_environments_status' do
-    subject { described_class.send(:build_environments_status, merge_request, user, sha) }
+    subject { described_class.send(:build_environments_status, merge_request, user, pipeline) }
 
     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) }
@@ -160,6 +156,39 @@
           expect(subject.count).to eq(0)
         end
       end
+
+      context 'when multiple deployments with the same SHA in different environments' do
+        let(:pipeline2) { create(:ci_pipeline, sha: sha, project: project) }
+        let!(:build2) { create(:ci_build, :start_review_app, pipeline: pipeline2) }
+
+        it 'returns deployments related to the head pipeline' 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 multiple deployments in the same pipeline for the same environments' do
+        let!(:build2) { create(:ci_build, :deploy_to_production, pipeline: pipeline) }
+
+        it 'returns unique entries' 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 stopped' do
+        before do
+          environment.stop!
+        end
+
+        it 'does not return environment status' do
+          expect(subject.count).to eq(0)
+        end
+      end
     end
   end
 end