Commit 5c896bf8 authored by Markus Koller's avatar Markus Koller

Rename `subject` to `__subject__` in delegated presenters

We want to encourage developers to use an explicit named getter instead
using `presents ..., as: <getter>`, since the `subject` name clashes
with some model attributes (like in the case of labels), and can be
confusing in general.

The `__subject__` getter can still be used for generic "internal" use,
for example when we need to unwrap a presenter in GraphQL's
`BaseResolver`.

This also removes the need for some `delegator_override` calls in
presenters.
parent da10c4a7
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......@@ -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
......
......@@ -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
......
......@@ -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)
)
......
......@@ -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
......
......@@ -21,7 +21,7 @@ module Gitlab
:project_blame_link,
:time_ago_tooltip)
def initialize(subject, **attributes)
def initialize(blame, **attributes)
super
@commits = {}
......
......@@ -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)
......
......@@ -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
......
......@@ -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
......@@ -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
......
......@@ -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
......
......@@ -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,17 +17,24 @@ 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)
expect(presenter.foo).to eq(project)
end
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) }
before do
presenter_class.class_eval do
presents Object, as: :foo
end
end
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
let(:presenter_class) do
Class.new(::Gitlab::View::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
......
......@@ -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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment