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

(cherry picked from commit 7674a8f477c90c1c8c9a969e7d80ea1ec9e72cd9)

e270fcc8 Fix deployment metrics in MR widget
15431054 Add spec for deployment metrics
2d6570f0 Do not remove the existing permission check
09e693c6 Add changelog
d8c24ac1 Remove unrelated changes

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 f87cc894d2f..19c1df25e18 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 52bd40ecb5e..5b3ebf0daa3 100644
--- a/spec/serializers/environment_status_entity_spec.rb
+++ b/spec/serializers/environment_status_entity_spec.rb
@@ -48,4 +48,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