Merge branch '341233-fix-subject-override-in-presenters' into 'master'

Rename `subject` to `__subject__` in delegated presenters

See merge request gitlab-org/gitlab!80908

app/controllers/clusters/clusters_controller.rb

@@ -184,7 +184,7 @@ class Clusters::ClustersController < Clusters::BaseController
   def cluster_list
     return [] unless certificate_based_clusters_enabled?
 
-    finder = ClusterAncestorsFinder.new(clusterable.subject, current_user)
+    finder = ClusterAncestorsFinder.new(clusterable.__subject__, current_user)
     clusters = finder.execute
 
     @has_ancestor_clusters = finder.has_ancestor_clusters?
@@ -253,7 +253,7 @@ class Clusters::ClustersController < Clusters::BaseController
       ]).merge(
         provider_type: :gcp,
         platform_type: :kubernetes,
-        clusterable: clusterable.subject
+        clusterable: clusterable.__subject__
       )
   end
 
@@ -274,7 +274,7 @@ class Clusters::ClustersController < Clusters::BaseController
       ]).merge(
         provider_type: :aws,
         platform_type: :kubernetes,
-        clusterable: clusterable.subject
+        clusterable: clusterable.__subject__
       )
   end
 
@@ -291,7 +291,7 @@ class Clusters::ClustersController < Clusters::BaseController
       ]).merge(
         provider_type: :user,
         platform_type: :kubernetes,
-        clusterable: clusterable.subject
+        clusterable: clusterable.__subject__
       )
   end
 
@@ -313,7 +313,7 @@ class Clusters::ClustersController < Clusters::BaseController
   end
 
   def gcp_cluster
-    cluster = Clusters::BuildService.new(clusterable.subject).execute
+    cluster = Clusters::BuildService.new(clusterable.__subject__).execute
     cluster.build_provider_gcp
     @gcp_cluster = cluster.present(current_user: current_user)
   end
@@ -343,7 +343,7 @@ class Clusters::ClustersController < Clusters::BaseController
   end
 
   def user_cluster
-    cluster = Clusters::BuildService.new(clusterable.subject).execute
+    cluster = Clusters::BuildService.new(clusterable.__subject__).execute
     cluster.build_platform_kubernetes
     @user_cluster = cluster.present(current_user: current_user)
   end

app/graphql/resolvers/base_resolver.rb

@@ -142,7 +142,7 @@ module Resolvers
     def object
       super.tap do |obj|
         # If the field this resolver is used in is wrapped in a presenter, unwrap its subject
-        break obj.subject if obj.is_a?(Gitlab::View::Presenter::Base)
+        break obj.__subject__ if obj.is_a?(Gitlab::View::Presenter::Base)
       end
     end
 

app/presenters/README.md

@@ -68,9 +68,11 @@ we gain the following benefits:
 
 If you need a presenter class that has only necessary interfaces for the view-related context,
 inherit from `Gitlab::View::Presenter::Simple`.
-It provides a `.presents` the method which allows you to define an accessor for the
-presented object. It also includes common helpers like `Gitlab::Routing` and
-`Gitlab::Allowable`.
+
+It provides a `.presents` class method which allows you to define the class the presenter is wrapping,
+and specify an accessor for the presented object using the `as:` keyword.
+
+It also includes common helpers like `Gitlab::Routing` and `Gitlab::Allowable`.
 
 ```ruby
 class LabelPresenter < Gitlab::View::Presenter::Simple

app/presenters/ci/bridge_presenter.rb

@@ -2,11 +2,11 @@
 
 module Ci
   class BridgePresenter < ProcessablePresenter
-    presents ::Ci::Bridge
+    presents ::Ci::Bridge, as: :bridge
 
     delegator_override :detailed_status
     def detailed_status
-      @detailed_status ||= subject.detailed_status(user)
+      @detailed_status ||= bridge.detailed_status(user)
     end
   end
 end

app/presenters/ci/build_presenter.rb

@@ -2,7 +2,7 @@
 
 module Ci
   class BuildPresenter < ProcessablePresenter
-    presents ::Ci::Build
+    presents ::Ci::Build, as: :build
 
     def erased_by_user?
       # Build can be erased through API, therefore it does not have
@@ -34,7 +34,7 @@ module Ci
     end
 
     def tooltip_message
-      "#{subject.name} - #{detailed_status.status_tooltip}"
+      "#{build.name} - #{detailed_status.status_tooltip}"
     end
 
     def execute_in
@@ -48,7 +48,7 @@ module Ci
     end
 
     def detailed_status
-      @detailed_status ||= subject.detailed_status(user)
+      @detailed_status ||= build.detailed_status(user)
     end
   end
 end

app/presenters/commit_status_presenter.rb

@@ -39,7 +39,7 @@ class CommitStatusPresenter < Gitlab::View::Presenter::Delegated
 
   private_constant :CALLOUT_FAILURE_MESSAGES
 
-  presents ::CommitStatus, as: :build
+  presents ::CommitStatus
 
   def self.callout_failure_messages
     CALLOUT_FAILURE_MESSAGES

app/presenters/dev_ops_report/metric_presenter.rb

@@ -2,28 +2,28 @@
 
 module DevOpsReport
   class MetricPresenter < Gitlab::View::Presenter::Simple
-    presents ::DevOpsReport::Metric
+    presents ::DevOpsReport::Metric, as: :metric
 
-    delegate :created_at, to: :subject
+    delegate :created_at, to: :metric
 
     def cards
       [
         Card.new(
-          metric: subject,
+          metric: metric,
           title: 'Issues',
           description: 'created per active user',
           feature: 'issues',
           blog: 'https://www2.deloitte.com/content/dam/Deloitte/se/Documents/technology-media-telecommunications/deloitte-digital-collaboration.pdf'
         ),
         Card.new(
-          metric: subject,
+          metric: metric,
           title: 'Comments',
           description: 'created per active user',
           feature: 'notes',
           blog: 'http://conversationaldevelopment.com/why/'
         ),
         Card.new(
-          metric: subject,
+          metric: metric,
           title: 'Milestones',
           description: 'created per active user',
           feature: 'milestones',
@@ -31,7 +31,7 @@ module DevOpsReport
           docs: help_page_path('user/project/milestones/index')
         ),
         Card.new(
-          metric: subject,
+          metric: metric,
           title: 'Boards',
           description: 'created per active user',
           feature: 'boards',
@@ -39,7 +39,7 @@ module DevOpsReport
           docs: help_page_path('user/project/issue_board')
         ),
         Card.new(
-          metric: subject,
+          metric: metric,
           title: 'Merge requests',
           description: 'per active user',
           feature: 'merge_requests',
@@ -47,7 +47,7 @@ module DevOpsReport
           docs: help_page_path('user/project/merge_requests/index')
         ),
         Card.new(
-          metric: subject,
+          metric: metric,
           title: 'Pipelines',
           description: 'created per active user',
           feature: 'ci_pipelines',
@@ -55,7 +55,7 @@ module DevOpsReport
           docs: help_page_path('ci/index')
         ),
         Card.new(
-          metric: subject,
+          metric: metric,
           title: 'Environments',
           description: 'created per active user',
           feature: 'environments',
@@ -63,14 +63,14 @@ module DevOpsReport
           docs: help_page_path('ci/environments')
         ),
         Card.new(
-          metric: subject,
+          metric: metric,
           title: 'Deployments',
           description: 'created per active user',
           feature: 'deployments',
           blog: 'https://puppet.com/blog/continuous-delivery-vs-continuous-deployment-what-s-diff'
         ),
         Card.new(
-          metric: subject,
+          metric: metric,
           title: 'Monitoring',
           description: 'fraction of all projects',
           feature: 'projects_prometheus_active',
@@ -78,7 +78,7 @@ module DevOpsReport
           docs: help_page_path('user/project/integrations/prometheus')
         ),
         Card.new(
-          metric: subject,
+          metric: metric,
           title: 'Service Desk',
           description: 'issues created per active user',
           feature: 'service_desk_issues',
@@ -91,52 +91,52 @@ module DevOpsReport
     def idea_to_production_steps
       [
         IdeaToProductionStep.new(
-          metric: subject,
+          metric: metric,
           title: 'Idea',
           features: %w(issues)
         ),
         IdeaToProductionStep.new(
-          metric: subject,
+          metric: metric,
           title: 'Issue',
           features: %w(issues notes)
         ),
         IdeaToProductionStep.new(
-          metric: subject,
+          metric: metric,
           title: 'Plan',
           features: %w(milestones boards)
         ),
         IdeaToProductionStep.new(
-          metric: subject,
+          metric: metric,
           title: 'Code',
           features: %w(merge_requests)
         ),
         IdeaToProductionStep.new(
-          metric: subject,
+          metric: metric,
           title: 'Commit',
           features: %w(merge_requests)
         ),
         IdeaToProductionStep.new(
-          metric: subject,
+          metric: metric,
           title: 'Test',
           features: %w(ci_pipelines)
         ),
         IdeaToProductionStep.new(
-          metric: subject,
+          metric: metric,
           title: 'Review',
           features: %w(ci_pipelines environments)
         ),
         IdeaToProductionStep.new(
-          metric: subject,
+          metric: metric,
           title: 'Staging',
           features: %w(environments deployments)
         ),
         IdeaToProductionStep.new(
-          metric: subject,
+          metric: metric,
           title: 'Production',
           features: %w(deployments)
         ),
         IdeaToProductionStep.new(
-          metric: subject,
+          metric: metric,
           title: 'Feedback',
           features: %w(projects_prometheus_active service_desk_issues)
         )

app/presenters/event_presenter.rb

@@ -3,7 +3,7 @@
 class EventPresenter < Gitlab::View::Presenter::Delegated
   presents ::Event, as: :event
 
-  def initialize(subject, **attributes)
+  def initialize(event, **attributes)
     super
 
     @visible_to_user_cache = ActiveSupport::Cache::MemoryStore.new

app/presenters/gitlab/blame_presenter.rb

@@ -21,7 +21,7 @@ module Gitlab
       :project_blame_link,
       :time_ago_tooltip)
 
-    def initialize(subject, **attributes)
+    def initialize(blame, **attributes)
       super
 
       @commits = {}

app/presenters/label_presenter.rb

@@ -4,8 +4,6 @@ class LabelPresenter < Gitlab::View::Presenter::Delegated
   presents ::Label, as: :label
   delegate :name, :full_name, to: :label_subject, prefix: :subject, allow_nil: true
 
-  delegator_override :subject # TODO: Fix `Gitlab::View::Presenter::Delegated#subject` not to override `Label#subject`.
-
   def edit_path
     case label
     when GroupLabel then edit_group_label_path(label.group, label)

app/presenters/pages_domain_presenter.rb

@@ -3,8 +3,6 @@
 class PagesDomainPresenter < Gitlab::View::Presenter::Delegated
   presents ::PagesDomain, as: :pages_domain
 
-  delegator_override :subject # TODO: Fix `Gitlab::View::Presenter::Delegated#subject` not to override `PagesDomain#subject`.
-
   def needs_verification?
     Gitlab::CurrentSettings.pages_domain_verification_enabled? && unverified?
   end

ee/app/presenters/approval_rule_presenter.rb

@@ -3,6 +3,8 @@
 class ApprovalRulePresenter < Gitlab::View::Presenter::Delegated
   include Gitlab::Utils::StrongMemoize
 
+  presents nil, as: :rule
+
   # Hide all approvers if any of them might come from a hidden group. This
   # represents an abundance of caution, but we can't tell which approvers come
   # from a hidden group and which don't, from here, so this is the simplest
@@ -26,6 +28,6 @@ class ApprovalRulePresenter < Gitlab::View::Presenter::Delegated
   private
 
   def group_query_service
-    @group_query_service ||= ApprovalRules::GroupFinder.new(@subject, current_user)
+    @group_query_service ||= ApprovalRules::GroupFinder.new(rule, current_user)
   end
 end

ee/app/presenters/merge_request_approver_presenter.rb

@@ -15,8 +15,8 @@ class MergeRequestApproverPresenter < Gitlab::View::Presenter::Simple
 
   attr_reader :skip_user
 
-  def initialize(subject, **attributes)
-    @skip_user = subject.author || attributes.delete(:skip_user)
+  def initialize(merge_request, **attributes)
+    @skip_user = merge_request.author || attributes.delete(:skip_user)
     super
   end
 

lib/gitlab/view/presenter/base.rb

@@ -11,15 +11,19 @@ module Gitlab
         include Gitlab::Routing
         include Gitlab::Allowable
 
-        attr_reader :subject
+        # Presenters should always access the subject through an explicit getter defined with
+        # `presents ..., as:`, the `__subject__` method is only intended for internal use.
+        def __subject__
+          @subject
+        end
 
         def can?(user, action, overridden_subject = nil)
-          super(user, action, overridden_subject || subject)
+          super(user, action, overridden_subject || __subject__)
         end
 
         # delegate all #can? queries to the subject
         def declarative_policy_delegate
-          subject
+          __subject__
         end
 
         def present(**attributes)
@@ -31,15 +35,15 @@ module Gitlab
         end
 
         def is_a?(type)
-          super || subject.is_a?(type)
+          super || __subject__.is_a?(type)
         end
 
         def web_url
-          url_builder.build(subject)
+          url_builder.build(__subject__)
         end
 
         def web_path
-          url_builder.build(subject, only_path: true)
+          url_builder.build(__subject__, only_path: true)
         end
 
         class_methods do
@@ -58,7 +62,7 @@ module Gitlab
               # no-op
             end
 
-            define_method(as) { subject } if as
+            define_method(as) { __subject__ } if as
           end
         end
       end

spec/lib/gitlab/view/presenter/base_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 RSpec.describe Gitlab::View::Presenter::Base do
   let(:project) { double(:project) }
   let(:presenter_class) do
-    Struct.new(:subject).include(described_class)
+    Struct.new(:__subject__).include(described_class)
   end
 
   describe '.presenter?' do
@@ -17,15 +17,22 @@ RSpec.describe Gitlab::View::Presenter::Base do
   end
 
   describe '.presents' do
-    it 'exposes #subject with the given keyword' do
-      presenter_class.presents(Object, as: :foo)
-      presenter = presenter_class.new(project)
+    it 'raises an error when symbol is passed' do
+      expect { presenter_class.presents(:foo) }.to raise_error(ArgumentError)
+    end
+
+    context 'when the presenter class specifies a custom keyword' do
+      subject(:presenter) { presenter_class.new(project) }
 
-      expect(presenter.foo).to eq(project)
+      before do
+        presenter_class.class_eval do
+          presents Object, as: :foo
+        end
       end
 
-    it 'raises an error when symbol is passed' do
-      expect { presenter_class.presents(:foo) }.to raise_error(ArgumentError)
+      it 'exposes the subject with the given keyword' do
+        expect(presenter.foo).to be(project)
+      end
     end
 
     context 'when the presenter class inherits Presenter::Delegated' do
@@ -50,13 +57,22 @@ RSpec.describe Gitlab::View::Presenter::Base do
       end
 
       it 'does not set the delegator target' do
-        expect(presenter_class).not_to receive(:delegator_target).with(Object)
+        expect(presenter_class).not_to receive(:delegator_target)
 
         presenter_class.presents(Object, as: :foo)
       end
     end
   end
 
+  describe '#__subject__' do
+    it 'returns the subject' do
+      subject = double
+      presenter = presenter_class.new(subject)
+
+      expect(presenter.__subject__).to be(subject)
+    end
+  end
+
   describe '#can?' do
     context 'user is not allowed' do
       it 'returns false' do

spec/presenters/ci/bridge_presenter_spec.rb

@@ -3,9 +3,10 @@
 require 'spec_helper'
 
 RSpec.describe Ci::BridgePresenter do
+  let_it_be(:user) { create(:user) }
   let_it_be(:project) { create(:project) }
   let_it_be(:pipeline) { create(:ci_pipeline, project: project) }
-  let_it_be(:bridge) { create(:ci_bridge, pipeline: pipeline, status: :failed) }
+  let_it_be(:bridge) { create(:ci_bridge, pipeline: pipeline, status: :failed, user: user) }
 
   subject(:presenter) do
     described_class.new(bridge)
@@ -14,4 +15,10 @@ RSpec.describe Ci::BridgePresenter do
   it 'presents information about recoverable state' do
     expect(presenter).to be_recoverable
   end
+
+  it 'presents the detailed status for the user' do
+    expect(bridge).to receive(:detailed_status).with(user)
+
+    presenter.detailed_status
+  end
 end