diff --git a/app/models/preloaders/environments/deployment_preloader.rb b/app/models/preloaders/environments/deployment_preloader.rb index fcf892698bbdab0e53ec9751e4cb1f084c918155..592d3e54a30c7188178dbf507cb4958c1e40fb9c 100644 --- a/app/models/preloaders/environments/deployment_preloader.rb +++ b/app/models/preloaders/environments/deployment_preloader.rb @@ -34,8 +34,16 @@ def load_deployment_association(association_name, association_attributes) deployments_by_environment_id = deployments.index_by(&:environment_id) environments.each do |environment| - environment.association(association_name).target = deployments_by_environment_id[environment.id] + associated_deployment = deployments_by_environment_id[environment.id] + + environment.association(association_name).target = associated_deployment environment.association(association_name).loaded! + + if associated_deployment + # `last?` in DeploymentEntity requires this environment to be loaded + associated_deployment.association(:environment).target = environment + associated_deployment.association(:environment).loaded! + end end end end diff --git a/doc/api/deployments.md b/doc/api/deployments.md index 29f9bd3e2ee3000bce0e2c092ff93c1afed279af..87ff65495403f0b31d32433e6efe6d02b5283c54 100644 --- a/doc/api/deployments.md +++ b/doc/api/deployments.md @@ -281,7 +281,8 @@ Deployments created by users on GitLab Premium or higher include the `approvals` "avatar_url": "https://www.gravatar.com/avatar/e83ac685f68ea07553ad3054c738c709?s=80&d=identicon", "web_url": "http://localhost:3000/project_6_bot" }, - "status": "approved" + "status": "approved", + "created_at": "2022-02-24T20:22:30.097Z" } ], ... @@ -351,7 +352,8 @@ Deployments created by users on GitLab Premium or higher include the `approvals` "avatar_url": "https://www.gravatar.com/avatar/e83ac685f68ea07553ad3054c738c709?s=80&d=identicon", "web_url": "http://localhost:3000/project_6_bot" }, - "status": "approved" + "status": "approved", + "created_at": "2022-02-24T20:22:30.097Z" } ], ... @@ -417,7 +419,8 @@ Deployments created by users on GitLab Premium or higher include the `approvals` "avatar_url": "https://www.gravatar.com/avatar/e83ac685f68ea07553ad3054c738c709?s=80&d=identicon", "web_url": "http://localhost:3000/project_6_bot" }, - "status": "approved" + "status": "approved", + "created_at": "2022-02-24T20:22:30.097Z" } ], ... @@ -480,6 +483,7 @@ Example response: "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon", "web_url": "http://localhost:3000/root" }, - "status": "approved" + "status": "approved", + "created_at": "2022-02-24T20:22:30.097Z" } ``` diff --git a/ee/app/models/ee/deployment.rb b/ee/app/models/ee/deployment.rb index e6ed0c411b5cdc03e66c01d180691d4b99c0de0e..2024d66f7929ae12c8f4cc721f5f948615fa7c79 100644 --- a/ee/app/models/ee/deployment.rb +++ b/ee/app/models/ee/deployment.rb @@ -34,7 +34,7 @@ module Deployment def pending_approval_count return 0 unless blocked? - environment.required_approval_count - approvals.count + environment.required_approval_count - approvals.length end end end diff --git a/ee/app/serializers/ee/deployment_entity.rb b/ee/app/serializers/ee/deployment_entity.rb index 54453d34d4eb0f2e0ecee388db09771c762ef977..84f8e4a3cd4b6c7ad7b388fea6035ddd394a802a 100644 --- a/ee/app/serializers/ee/deployment_entity.rb +++ b/ee/app/serializers/ee/deployment_entity.rb @@ -6,6 +6,7 @@ module DeploymentEntity prepended do expose :pending_approval_count + expose :approvals, using: ::API::Entities::Deployments::Approval end end end diff --git a/ee/app/serializers/ee/environment_serializer.rb b/ee/app/serializers/ee/environment_serializer.rb index 93247cf07a3d232aea93ca32410fd7037fcb5ad5..f5a31df6a34eb8d3790793cc3aa034ba3293879a 100644 --- a/ee/app/serializers/ee/environment_serializer.rb +++ b/ee/app/serializers/ee/environment_serializer.rb @@ -10,6 +10,13 @@ def environment_associations super.deep_merge(latest_opened_most_severe_alert: []) end + override :deployment_associations + def deployment_associations + super.deep_merge(approvals: { + user: [] + }) + end + override :project_associations def project_associations super.deep_merge(protected_environments: []) diff --git a/ee/lib/api/entities/deployments/approval.rb b/ee/lib/api/entities/deployments/approval.rb index 102e237f568fcdf39af956bf44cd2baa54e11e95..a24c4910a477cf163bbfbf9270ace7fcc7e708af 100644 --- a/ee/lib/api/entities/deployments/approval.rb +++ b/ee/lib/api/entities/deployments/approval.rb @@ -6,6 +6,7 @@ module Deployments class Approval < Grape::Entity expose :user, using: Entities::UserBasic expose :status + expose :created_at end end end diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/deployment_approval.json b/ee/spec/fixtures/api/schemas/public_api/v4/deployment_approval.json index 517454d1709dc82cd2bb8db76a9331913c7129d0..469b1c9142c6d1a3b835b7d7c4600c728427b4dd 100644 --- a/ee/spec/fixtures/api/schemas/public_api/v4/deployment_approval.json +++ b/ee/spec/fixtures/api/schemas/public_api/v4/deployment_approval.json @@ -13,6 +13,10 @@ "items": { "$ref": "../../../../../../../spec/fixtures/api/schemas/public_api/v4/user/basic.json" } + }, + "created_at": { + "type": "string", + "format": "date-time" } }, "additionalProperties": false diff --git a/ee/spec/lib/api/entities/deployments/approval_spec.rb b/ee/spec/lib/api/entities/deployments/approval_spec.rb index 3c8dd1f07bc590456731cd0b515eb4e4b664a7a8..ec7d138f3e69cd5fb3ceaeece243ac7ed0bdb3b3 100644 --- a/ee/spec/lib/api/entities/deployments/approval_spec.rb +++ b/ee/spec/lib/api/entities/deployments/approval_spec.rb @@ -8,6 +8,6 @@ let(:approval) { build(:deployment_approval) } it 'exposes correct attributes' do - expect(subject.keys).to contain_exactly(:user, :status) + expect(subject.keys).to contain_exactly(:user, :status, :created_at) end end diff --git a/ee/spec/models/deployment_spec.rb b/ee/spec/models/deployment_spec.rb index 9cfdc56d6a8922a61efe875be9bed2bfc726ad3b..1d37a9a339e9cf6393d0bcf9a1793d69145ff077 100644 --- a/ee/spec/models/deployment_spec.rb +++ b/ee/spec/models/deployment_spec.rb @@ -69,6 +69,17 @@ expect(deployment.pending_approval_count).to eq(0) end end + + context 'loading approval count' do + before do + deployment.environment.required_approval_count + deployment.approvals.to_a + end + + it 'does not perform an extra query when approvals are loaded', :request_store do + expect { deployment.pending_approval_count }.not_to exceed_query_limit(0) + end + end end context 'when Protected Environments feature is not available' do diff --git a/ee/spec/serializers/ee/deployment_entity_spec.rb b/ee/spec/serializers/ee/deployment_entity_spec.rb index a4026210390710d22cd03cb1847998577c72ccc0..e2b91ea983a82acbb7d5038b2793eee300428f10 100644 --- a/ee/spec/serializers/ee/deployment_entity_spec.rb +++ b/ee/spec/serializers/ee/deployment_entity_spec.rb @@ -21,4 +21,10 @@ expect(subject[:pending_approval_count]).to eq(2) end end + + describe '#approvals' do + it 'exposes approvals' do + expect(subject[:approvals].length).to eq(1) + end + end end diff --git a/ee/spec/serializers/ee/environment_serializer_spec.rb b/ee/spec/serializers/ee/environment_serializer_spec.rb index aee337363e5ee54be045b4088f2cc512e243a45c..0a01582adf6b4731144ec87e0f4afbcd7c5fee0f 100644 --- a/ee/spec/serializers/ee/environment_serializer_spec.rb +++ b/ee/spec/serializers/ee/environment_serializer_spec.rb @@ -19,8 +19,10 @@ def create_environment_with_associations(project) create(:environment, project: project).tap do |environment| create(:deployment, :success, environment: environment, project: project) - create(:deployment, :running, environment: environment, project: project) - create(:protected_environment, :maintainers_can_deploy, name: environment.name, project: project) + create(:deployment, :blocked, environment: environment, project: project) do |deployment| + create(:deployment_approval, deployment: deployment) + end + create(:protected_environment, :maintainers_can_deploy, name: environment.name, project: project, required_approval_count: 2) prometheus_alert = create(:prometheus_alert, project: project, environment: environment) create(:alert_management_alert, :triggered, :prometheus, project: project, environment: environment, prometheus_alert: prometheus_alert) end diff --git a/spec/models/preloaders/environments/deployment_preloader_spec.rb b/spec/models/preloaders/environments/deployment_preloader_spec.rb index c1812d45628771472ff396f0b757dd3c66b9fee4..3f2f28a069e6c3cfe65db9dd9e753b7401c883a4 100644 --- a/spec/models/preloaders/environments/deployment_preloader_spec.rb +++ b/spec/models/preloaders/environments/deployment_preloader_spec.rb @@ -62,4 +62,22 @@ def deployment_associations expect(default_preload_query).to be(false) end + + it 'sets environment on the associated deployment', :aggregate_failures do + preload_association(:last_deployment) + + expect do + project.environments.each { |environment| environment.last_deployment.environment } + end.not_to exceed_query_limit(0) + + project.environments.each do |environment| + expect(environment.last_deployment.environment).to eq(environment) + end + end + + it 'does not attempt to set environment on a nil deployment' do + create(:environment, project: project, state: :available) + + expect { preload_association(:last_deployment) }.not_to raise_error + end end diff --git a/spec/support/shared_examples/serializers/environment_serializer_shared_examples.rb b/spec/support/shared_examples/serializers/environment_serializer_shared_examples.rb index ad4057a972a9b2f35c1e61696083e903dfbac9a1..87a330604353e93cf4078c45033ee75b2def7193 100644 --- a/spec/support/shared_examples/serializers/environment_serializer_shared_examples.rb +++ b/spec/support/shared_examples/serializers/environment_serializer_shared_examples.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true RSpec.shared_examples 'avoid N+1 on environments serialization' do |ee: false| # Investigating in https://gitlab.com/gitlab-org/gitlab/-/issues/353209 - let(:query_threshold) { 9 + (ee ? 4 : 0) } + let(:query_threshold) { 1 + (ee ? 4 : 0) } it 'avoids N+1 database queries with grouping', :request_store do create_environment_with_associations(project)