Merge branch 'fix-deployment-metrics-in-mr-widget' into 'master'

Avoid returning deployment metrics url to MR widget when the deployment is not successful

Closes #53870

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

4 files changed, 50 insertions, 4 deletions

diff --git a/app/models/deployment.rb b/app/models/deployment.rb
index 83434276995..811e623b7f7 100644
--- a/app/models/deployment.rb
+++ b/app/models/deployment.rb
@@ -160,18 +160,18 @@ class Deployment < ActiveRecord::Base
   end
 
   def has_metrics?
-    prometheus_adapter&.can_query?
+    prometheus_adapter&.can_query? && success?
   end
 
   def metrics
-    return {} unless has_metrics? && success?
+    return {} unless has_metrics?
 
     metrics = prometheus_adapter.query(:deployment, self)
     metrics&.merge(deployment_time: finished_at.to_i) || {}
   end
 
   def additional_metrics
-    return {} unless has_metrics? && success?
+    return {} unless has_metrics?
 
     metrics = prometheus_adapter.query(:additional_metrics_deployment, self)
     metrics&.merge(deployment_time: finished_at.to_i) || {}
diff --git a/app/serializers/environment_status_entity.rb b/app/serializers/environment_status_entity.rb
index 4c6664e9e25..f6321b9e520 100644
--- a/app/serializers/environment_status_entity.rb
+++ b/app/serializers/environment_status_entity.rb
@@ -11,7 +11,7 @@ class EnvironmentStatusEntity < Grape::Entity
     project_environment_path(es.project, es.environment)
   end
 
-  expose :metrics_url, if: ->(*) { can_read_environment? && environment.has_metrics? } do |es|
+  expose :metrics_url, if: ->(*) { can_read_environment? && deployment.has_metrics? } do |es|
     metrics_project_environment_deployment_path(es.project, es.environment, es.deployment)
   end
 
@@ -45,6 +45,10 @@ class EnvironmentStatusEntity < Grape::Entity
     object.environment
   end
 
+  def deployment
+    object.deployment
+  end
+
   def project
     object.environment.project
   end
diff --git a/changelogs/unreleased/fix-deployment-metrics-in-mr-widget.yml b/changelogs/unreleased/fix-deployment-metrics-in-mr-widget.yml
new file mode 100644
index 00000000000..5427ead3d1b
--- /dev/null
+++ b/changelogs/unreleased/fix-deployment-metrics-in-mr-widget.yml
@@ -0,0 +1,6 @@
+---
+title: Avoid returning deployment metrics url to MR widget when the deployment is
+  not successful
+merge_request: 23010
+author:
+type: fixed
diff --git a/spec/serializers/environment_status_entity_spec.rb b/spec/serializers/environment_status_entity_spec.rb
index 58e2e627410..8a6a38fe5f8 100644
--- a/spec/serializers/environment_status_entity_spec.rb
+++ b/spec/serializers/environment_status_entity_spec.rb
@@ -40,4 +40,40 @@ describe EnvironmentStatusEntity do
 
     it { is_expected.to include(:stop_url) }
   end
+
+  context 'when deployment has metrics' do
+    let(:prometheus_adapter) { double('prometheus_adapter', can_query?: true) }
+
+    let(:simple_metrics) do
+      {
+        success: true,
+        metrics: {},
+        last_update: 42
+      }
+    end
+
+    before do
+      project.add_maintainer(user)
+      allow(deployment).to receive(:prometheus_adapter).and_return(prometheus_adapter)
+      allow(prometheus_adapter).to receive(:query).with(:deployment, deployment).and_return(simple_metrics)
+      allow(entity).to receive(:deployment).and_return(deployment)
+    end
+
+    context 'when deployment succeeded' do
+      let(:deployment)    { create(:deployment, :succeed, :review_app) }
+
+      it 'returns metrics url' do
+        expect(subject[:metrics_url])
+          .to eq("/#{project.namespace.name}/#{project.name}/environments/#{environment.id}/deployments/#{deployment.iid}/metrics")
+      end
+    end
+
+    context 'when deployment is running' do
+      let(:deployment)    { create(:deployment, :running, :review_app) }
+
+      it 'does not return metrics url' do
+        expect(subject[:metrics_url]).to be_nil
+      end
+    end
+  end
 end