Merge branch '23563-document-presenters' into 'master'

Document presenters

Closes #23563

See merge request !8480

app/controllers/projects/builds_controller.rb

@@ -94,7 +94,7 @@ class Projects::BuildsController < Projects::ApplicationController
   private
 
   def build
-    @build ||= project.builds.find_by!(id: params[:id])
+    @build ||= project.builds.find_by!(id: params[:id]).present(user: current_user)
   end
 
   def build_path(build)

app/models/ci/build.rb

@@ -2,6 +2,7 @@ module Ci
   class Build < CommitStatus
     include TokenAuthenticatable
     include AfterCommitQueue
+    include Presentable
 
     belongs_to :runner
     belongs_to :trigger_request

app/models/concerns/presentable.rb

@@ -0,0 +1,7 @@
+module Presentable
+  def present(**attributes)
+    Gitlab::View::Presenter::Factory
+      .new(self, attributes)
+      .fabricate!
+  end
+end

app/policies/base_policy.rb

@@ -53,6 +53,10 @@ class BasePolicy
   def self.class_for(subject)
     return GlobalPolicy if subject.nil?
 
+    if subject.class.try(:presenter?)
+      subject = subject.subject
+    end
+
     subject.class.ancestors.each do |klass|
       next unless klass.name
 

app/presenters/README.md

@@ -0,0 +1,154 @@
+# Presenters
+
+This type of class is responsible for giving the view an object which defines
+**view-related logic/data methods**. It is usually useful to extract such
+methods from models to presenters.
+
+## When to use a presenter?
+
+### When your view is full of logic
+
+When your view is full of logic (`if`, `else`, `select` on arrays etc.), it's
+time to create a presenter!
+
+### When your model has a lot of view-related logic/data methods
+
+When your model has a lot of view-related logic/data methods, you can easily
+move them to a presenter.
+
+## Why are we using presenters instead of helpers?
+
+We don't use presenters to generate complex view output that would rely on helpers.
+
+Presenters should be used for:
+
+- Data and logic methods that can be pulled & combined into single methods from
+  view. This can include loops extracted from views too. A good example is
+  https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7073/diffs.
+- Data and logic methods that can be pulled from models.
+- Simple text output methods: it's ok if the method returns a string, but not a
+  whole DOM element for which we'd need HAML, a view context, helpers etc.
+
+## Why use presenters instead of model concerns?
+
+We should strive to follow the single-responsibility principle, and view-related
+logic/data methods are definitely not the responsibility of models!
+
+Another reason is as follows:
+
+> Avoid using concerns and use presenters instead. Why? After all, concerns seem
+to be a core part of Rails and can DRY up code when shared among multiple models.
+Nonetheless, the main issue is that concerns don’t make the model object more
+cohesive. The code is just better organized. In other words, there’s no real
+change to the API of the model.
+
+– https://www.toptal.com/ruby-on-rails/decoupling-rails-components
+
+## Benefits
+
+By moving pure view-related logic/data methods from models & views to presenters,
+we gain the following benefits:
+
+- rules are more explicit and centralized in the presenter => improves security
+- testing is easier and faster as presenters are Plain Old Ruby Object (PORO)
+- views are more readable and maintainable
+- decreases number of CE -> EE merge conflicts since code is in separate files
+- moves the conflicts from views (not always obvious) to presenters (a lot easier to resolve)
+
+## What not to do with presenters?
+
+- Don't use helpers in presenters. Presenters are not aware of the view context.
+- Don't generate complex DOM elements, forms etc. with presenters. Presenters
+  can return simple data as texts, and URLs using URL helpers from
+  `Gitlab::Routing` but nothing much more fancy.
+
+## Implementation
+
+### Presenter definition
+
+Every presenter should inherit from `Gitlab::View::Presenter::Simple`, which
+provides a `.presents` method which allows you to define an accessor for the
+presented object. It also includes common helpers like `Gitlab::Routing` and
+`Gitlab::Allowable`.
+
+```ruby
+class LabelPresenter < Gitlab::View::Presenter::Simple
+  presents :label
+
+  def text_color
+    label.color.to_s
+  end
+
+  def to_partial_path
+    'projects/labels/show'
+  end
+end
+```
+
+In some cases, it can be more practical to transparently delegate all missing
+method calls to the presented object, in these cases, you can make your
+presenter inherit from `Gitlab::View::Presenter::Delegated`:
+
+```ruby
+class LabelPresenter < Gitlab::View::Presenter::Delegated
+  presents :label
+
+  def text_color
+    # color is delegated to label
+    color.to_s
+  end
+
+  def to_partial_path
+    'projects/labels/show'
+  end
+end
+```
+
+### Presenter instantiation
+
+Instantiation must be done via the `Gitlab::View::Presenter::Factory` class which
+detects the presenter based on the presented subject's class.
+
+```ruby
+class Projects::LabelsController < Projects::ApplicationController
+  def edit
+    @label = Gitlab::View::Presenter::Factory
+      .new(@label, user: current_user)
+      .fabricate!
+  end
+end
+```
+
+You can also include the `Presentable` concern in the model:
+
+```ruby
+class Label
+  include Presentable
+end
+```
+
+and then in the controller:
+
+```ruby
+class Projects::LabelsController < Projects::ApplicationController
+  def edit
+    @label = @label.present(user: current_user)
+  end
+end
+```
+
+### Presenter usage
+
+```ruby
+%div{ class: @label.text_color }
+  = render partial: @label, label: @label
+```
+
+You can also present the model in the view:
+
+```ruby
+- label = @label.present(current_user)
+
+%div{ class: label.text_color }
+  = render partial: label, label: label
+```

app/presenters/ci/build_presenter.rb

@@ -0,0 +1,15 @@
+module Ci
+  class BuildPresenter < Gitlab::View::Presenter::Delegated
+    presents :build
+
+    def erased_by_user?
+      # Build can be erased through API, therefore it does not have
+      # `erased_by` user assigned in that case.
+      erased? && erased_by
+    end
+
+    def erased_by_name
+      erased_by.name if erased_by_user?
+    end
+  end
+end

app/views/projects/builds/show.html.haml

@@ -51,8 +51,10 @@
     .prepend-top-default
       - if @build.erased?
         .erased.alert.alert-warning
-          - erased_by = "by #{link_to @build.erased_by.name, user_path(@build.erased_by)}" if @build.erased_by
-          Build has been erased #{erased_by.html_safe} #{time_ago_with_tooltip(@build.erased_at)}
+          - if @build.erased_by_user?
+            Build has been erased by #{link_to(@build.erased_by_name, user_path(@build.erased_by))} #{time_ago_with_tooltip(@build.erased_at)}
+          - else
+            Build has been erased #{time_ago_with_tooltip(@build.erased_at)}
       - else
         #js-build-scroll.scroll-controls
           .scroll-step

lib/gitlab/view/presenter/base.rb

@@ -0,0 +1,28 @@
+module Gitlab
+  module View
+    module Presenter
+      module Base
+        extend ActiveSupport::Concern
+
+        include Gitlab::Routing
+        include Gitlab::Allowable
+
+        attr_reader :subject
+
+        def can?(user, action, overriden_subject = nil)
+          super(user, action, overriden_subject || subject)
+        end
+
+        class_methods do
+          def presenter?
+            true
+          end
+
+          def presents(name)
+            define_method(name) { subject }
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/view/presenter/delegated.rb

@@ -0,0 +1,19 @@
+module Gitlab
+  module View
+    module Presenter
+      class Delegated < SimpleDelegator
+        include Gitlab::View::Presenter::Base
+
+        def initialize(subject, **attributes)
+          @subject = subject
+
+          attributes.each do |key, value|
+            define_singleton_method(key) { value }
+          end
+
+          super(subject)
+        end
+      end
+    end
+  end
+end

lib/gitlab/view/presenter/factory.rb

@@ -0,0 +1,24 @@
+module Gitlab
+  module View
+    module Presenter
+      class Factory
+        def initialize(subject, **attributes)
+          @subject = subject
+          @attributes = attributes
+        end
+
+        def fabricate!
+          presenter_class.new(subject, attributes)
+        end
+
+        private
+
+        attr_reader :subject, :attributes
+
+        def presenter_class
+          "#{subject.class.name}Presenter".constantize
+        end
+      end
+    end
+  end
+end

lib/gitlab/view/presenter/simple.rb

@@ -0,0 +1,17 @@
+module Gitlab
+  module View
+    module Presenter
+      class Simple
+        include Gitlab::View::Presenter::Base
+
+        def initialize(subject, **attributes)
+          @subject = subject
+
+          attributes.each do |key, value|
+            define_singleton_method(key) { value }
+          end
+        end
+      end
+    end
+  end
+end

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

@@ -0,0 +1,51 @@
+require 'spec_helper'
+
+describe Gitlab::View::Presenter::Base do
+  let(:project) { double(:project) }
+  let(:presenter_class) do
+    Struct.new(:subject).include(described_class)
+  end
+
+  describe '.presenter?' do
+    it 'returns true' do
+      presenter = presenter_class.new(project)
+
+      expect(presenter.class).to be_presenter
+    end
+  end
+
+  describe '.presents' do
+    it 'exposes #subject with the given keyword' do
+      presenter_class.presents(:foo)
+      presenter = presenter_class.new(project)
+
+      expect(presenter.foo).to eq(project)
+    end
+  end
+
+  describe '#can?' do
+    context 'user is not allowed' do
+      it 'returns false' do
+        presenter = presenter_class.new(build_stubbed(:empty_project))
+
+        expect(presenter.can?(nil, :read_project)).to be_falsy
+      end
+    end
+
+    context 'user is allowed' do
+      it 'returns true' do
+        presenter = presenter_class.new(build_stubbed(:empty_project, :public))
+
+        expect(presenter.can?(nil, :read_project)).to be_truthy
+      end
+    end
+
+    context 'subject is overriden' do
+      it 'returns true' do
+        presenter = presenter_class.new(build_stubbed(:empty_project, :public))
+
+        expect(presenter.can?(nil, :read_project, build_stubbed(:empty_project))).to be_falsy
+      end
+    end
+  end
+end

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

@@ -0,0 +1,29 @@
+require 'spec_helper'
+
+describe Gitlab::View::Presenter::Delegated do
+  let(:project) { double(:project, bar: 'baz') }
+  let(:presenter_class) do
+    Class.new(described_class)
+  end
+
+  it 'includes Gitlab::View::Presenter::Base' do
+    expect(described_class).to include(Gitlab::View::Presenter::Base)
+  end
+
+  describe '#initialize' do
+    it 'takes arbitrary key/values and exposes them' do
+      presenter = presenter_class.new(project, user: 'user', foo: 'bar')
+
+      expect(presenter.user).to eq('user')
+      expect(presenter.foo).to eq('bar')
+    end
+  end
+
+  describe 'delegation' do
+    it 'forwards missing methods to subject' do
+      presenter = presenter_class.new(project)
+
+      expect(presenter.bar).to eq('baz')
+    end
+  end
+end

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

@@ -0,0 +1,38 @@
+require 'spec_helper'
+
+describe Gitlab::View::Presenter::Factory do
+  let(:build) { Ci::Build.new }
+
+  describe '#initialize' do
+    context 'without optional parameters' do
+      it 'takes a subject and optional params' do
+        presenter = described_class.new(build)
+
+        expect { presenter }.not_to raise_error
+      end
+    end
+
+    context 'with optional parameters' do
+      it 'takes a subject and optional params' do
+        presenter = described_class.new(build, user: 'user')
+
+        expect { presenter }.not_to raise_error
+      end
+    end
+  end
+
+  describe '#fabricate!' do
+    it 'exposes given params' do
+      presenter = described_class.new(build, user: 'user', foo: 'bar').fabricate!
+
+      expect(presenter.user).to eq('user')
+      expect(presenter.foo).to eq('bar')
+    end
+
+    it 'detects the presenter based on the given subject' do
+      presenter = described_class.new(build).fabricate!
+
+      expect(presenter).to be_a(Ci::BuildPresenter)
+    end
+  end
+end

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

@@ -0,0 +1,29 @@
+require 'spec_helper'
+
+describe Gitlab::View::Presenter::Simple do
+  let(:project) { double(:project) }
+  let(:presenter_class) do
+    Class.new(described_class)
+  end
+
+  it 'includes Gitlab::View::Presenter::Base' do
+    expect(described_class).to include(Gitlab::View::Presenter::Base)
+  end
+
+  describe '#initialize' do
+    it 'takes arbitrary key/values and exposes them' do
+      presenter = presenter_class.new(project, user: 'user', foo: 'bar')
+
+      expect(presenter.user).to eq('user')
+      expect(presenter.foo).to eq('bar')
+    end
+  end
+
+  describe 'delegation' do
+    it 'does not forward missing methods to subject' do
+      presenter = presenter_class.new(project)
+
+      expect { presenter.foo }.to raise_error(NoMethodError)
+    end
+  end
+end

spec/models/concerns/presentable_spec.rb

@@ -0,0 +1,15 @@
+require 'spec_helper'
+
+describe Presentable do
+  let(:build) { Ci::Build.new }
+
+  describe '#present' do
+    it 'returns a presenter' do
+      expect(build.present).to be_a(Ci::BuildPresenter)
+    end
+
+    it 'takes optional attributes' do
+      expect(build.present(foo: 'bar').foo).to eq('bar')
+    end
+  end
+end

spec/policies/base_policy_spec.rb

@@ -0,0 +1,17 @@
+require 'spec_helper'
+
+describe BasePolicy, models: true do
+  let(:build) { Ci::Build.new }
+
+  describe '.class_for' do
+    it 'detects policy class based on the subject ancestors' do
+      expect(described_class.class_for(build)).to eq(Ci::BuildPolicy)
+    end
+
+    it 'detects policy class for a presented subject' do
+      presentee = Ci::BuildPresenter.new(build)
+
+      expect(described_class.class_for(presentee)).to eq(Ci::BuildPolicy)
+    end
+  end
+end

spec/presenters/ci/build_presenter_spec.rb

@@ -0,0 +1,77 @@
+require 'spec_helper'
+
+describe Ci::BuildPresenter do
+  let(:project) { create(:empty_project) }
+  let(:pipeline) { create(:ci_pipeline, project: project) }
+  let(:build) { create(:ci_build, pipeline: pipeline) }
+
+  subject(:presenter) do
+    described_class.new(build)
+  end
+
+  it 'inherits from Gitlab::View::Presenter::Delegated' do
+    expect(described_class.superclass).to eq(Gitlab::View::Presenter::Delegated)
+  end
+
+  describe '#initialize' do
+    it 'takes a build and optional params' do
+      expect { presenter }.not_to raise_error
+    end
+
+    it 'exposes build' do
+      expect(presenter.build).to eq(build)
+    end
+
+    it 'forwards missing methods to build' do
+      expect(presenter.ref).to eq('master')
+    end
+  end
+
+  describe '#erased_by_user?' do
+    it 'takes a build and optional params' do
+      expect(presenter).not_to be_erased_by_user
+    end
+  end
+
+  describe '#erased_by_name' do
+    context 'when build is not erased' do
+      before do
+        expect(presenter).to receive(:erased_by_user?).and_return(false)
+      end
+
+      it 'returns nil' do
+        expect(presenter.erased_by_name).to be_nil
+      end
+    end
+
+    context 'when build is erased' do
+      before do
+        expect(presenter).to receive(:erased_by_user?).and_return(true)
+        expect(build).to receive(:erased_by).
+          and_return(double(:user, name: 'John Doe'))
+      end
+
+      it 'returns the name of the eraser' do
+        expect(presenter.erased_by_name).to eq('John Doe')
+      end
+    end
+  end
+
+  describe 'quack like a Ci::Build permission-wise' do
+    context 'user is not allowed' do
+      let(:project) { build_stubbed(:empty_project, public_builds: false) }
+
+      it 'returns false' do
+        expect(presenter.can?(nil, :read_build)).to be_falsy
+      end
+    end
+
+    context 'user is allowed' do
+      let(:project) { build_stubbed(:empty_project, :public) }
+
+      it 'returns true' do
+        expect(presenter.can?(nil, :read_build)).to be_truthy
+      end
+    end
+  end
+end