diff options
author | Kamil Trzciński <ayufan@ayufan.eu> | 2018-12-03 10:55:29 +0000 |
---|---|---|
committer | Kamil Trzciński <ayufan@ayufan.eu> | 2018-12-03 10:55:29 +0000 |
commit | 8a465b0c456e4f774f02ec1991d32549872f5684 (patch) | |
tree | 9d29352ed55081e6e3cd995fbf79dac9e09a5741 | |
parent | 76d4e6d6d87f3e59f1864fa15da701bb789e301e (diff) | |
parent | 4bd00e5378736231225394f80b87b7f89077cb76 (diff) | |
download | gitlab-ce-8a465b0c456e4f774f02ec1991d32549872f5684.tar.gz |
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
-rw-r--r-- | app/models/ci/pipeline.rb | 6 | ||||
-rw-r--r-- | app/models/environment_status.rb | 14 | ||||
-rw-r--r-- | changelogs/unreleased/fix-mr-widget-unrelated-deployment-status.yml | 5 | ||||
-rw-r--r-- | spec/features/merge_request/user_sees_deployment_widget_spec.rb | 16 | ||||
-rw-r--r-- | spec/lib/gitlab/import_export/all_models.yml | 2 | ||||
-rw-r--r-- | spec/models/environment_status_spec.rb | 39 |
6 files changed, 66 insertions, 16 deletions
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 9512ba42f67..1487b9d3bca 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -26,6 +26,8 @@ module Ci 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 @@ module Ci 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 diff --git a/app/models/environment_status.rb b/app/models/environment_status.rb index 4a128dde5cd..2fb6cadc8cd 100644 --- a/app/models/environment_status.rb +++ b/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 @@ class EnvironmentStatus } 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 diff --git a/changelogs/unreleased/fix-mr-widget-unrelated-deployment-status.yml b/changelogs/unreleased/fix-mr-widget-unrelated-deployment-status.yml new file mode 100644 index 00000000000..ab926fbd43b --- /dev/null +++ b/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 diff --git a/spec/features/merge_request/user_sees_deployment_widget_spec.rb b/spec/features/merge_request/user_sees_deployment_widget_spec.rb index 3e40179ad9a..fe8e0b07d39 100644 --- a/spec/features/merge_request/user_sees_deployment_widget_spec.rb +++ b/spec/features/merge_request/user_sees_deployment_widget_spec.rb @@ -29,6 +29,22 @@ describe 'Merge request > User sees deployment widget', :js do 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 diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 8d2f60d7a8b..31ab11bbf8d 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -121,6 +121,8 @@ pipelines: - artifacts - pipeline_schedule - merge_requests +- deployments +- environments pipeline_variables: - pipeline stages: diff --git a/spec/models/environment_status_spec.rb b/spec/models/environment_status_spec.rb index 90f7e4a4590..9da16dea929 100644 --- a/spec/models/environment_status_spec.rb +++ b/spec/models/environment_status_spec.rb @@ -92,16 +92,12 @@ describe EnvironmentStatus do 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 @@ describe EnvironmentStatus do 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 |