Document presenters

Signed-off-by: Rémy Coutable <remy@rymai.me>

9 files changed, 391 insertions, 0 deletions

diff --git a/app/presenters/README.md b/app/presenters/README.md
new file mode 100644
index 00000000000..38399b5e18b
--- /dev/null
+++ b/app/presenters/README.md
@@ -0,0 +1,165 @@
+# 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!
+
+For instance this view is full of logic: https://gitlab.com/gitlab-org/gitlab-ce/blob/d61f8a18e0f7e9d0ed162827f4e8ae2de3756f5c/app/views/projects/builds/_sidebar.html.haml
+can be improved as follows: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7073/diffs
+
+### 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 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 using 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
+- makes the testing easier & faster as presenters are Plain Old Ruby Object (PORO)
+- makes views much 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 URL using URL helpers from
+  `Gitlab::Routing` but nothing much more fancy.
+
+## Implementation
+
+### Presenter definition
+
+Every presenters should include `Gitlab::View::Presenter`, 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
+  include Gitlab::View::Presenter
+
+  presents :label
+
+  def blue?
+    label.color == :blue
+  end
+
+  def to_partial_path
+    'projects/labels/show'
+  end
+end
+```
+
+In some cases, it can be more practical to transparently delegates all missing
+method calls to the presented object, in these cases, you can make your
+presenter inherit from `SimpleDelegator`:
+
+```ruby
+class LabelPresenter < SimpleDelegator
+  include Gitlab::View::Presenter
+
+  presents :label
+
+  def blue?
+    # color is delegated to label
+    color == :blue
+  end
+
+  def to_partial_path
+    'projects/labels/show'
+  end
+end
+```
+
+### Presenter instantiation
+
+Instantiation must be done via the `Gitlab::View::PresenterFactory` class which
+handles presenters subclassing `SimpleDelegator` as well as those who don't.
+
+```ruby
+class Projects::LabelsController < Projects::ApplicationController
+  def edit
+    @label = Gitlab::View::PresenterFactory
+      .new(@label, user: current_user)
+      .fabricate!
+  end
+end
+```
+
+You can also define a method on the model:
+
+```ruby
+class Label
+  def present(current_user)
+    Gitlab::View::PresenterFactory
+      .new(self, user: current_user)
+      .fabricate!
+  end
+end
+```
+
+and then in the controller:
+
+```ruby
+class Projects::LabelsController < Projects::ApplicationController
+  def edit
+    @label = @label.present(current_user)
+  end
+end
+```
+
+### Presenter usage
+
+```ruby
+= @label.blue?
+
+= render partial: @label, label: @label
+```
+
+You can also present the model in the view:
+
+```ruby
+- label = @label.present(current_user)
+
+= render partial: label, label: label
+```
diff --git a/app/presenters/build_presenter.rb b/app/presenters/build_presenter.rb
new file mode 100644
index 00000000000..9c44a6d2dbe
--- /dev/null
+++ b/app/presenters/build_presenter.rb
@@ -0,0 +1,15 @@
+class BuildPresenter < SimpleDelegator
+  include Gitlab::View::Presenter
+
+  presents :build
+
+  def erased_by_user?
+    # Build can be erased through API, therefore it does not have
+    # `erase_by` user assigned in that case.
+    erased? && erased_by
+  end
+
+  def self.ancestors
+    super + [Ci::Build]
+  end
+end
diff --git a/app/presenters/variable_presenter.rb b/app/presenters/variable_presenter.rb
new file mode 100644
index 00000000000..80382f3a001
--- /dev/null
+++ b/app/presenters/variable_presenter.rb
@@ -0,0 +1,5 @@
+class VariablePresenter
+  include Gitlab::View::Presenter
+
+  presents :variable
+end
diff --git a/lib/gitlab/view/presenter.rb b/lib/gitlab/view/presenter.rb
new file mode 100644
index 00000000000..8b63b271b99
--- /dev/null
+++ b/lib/gitlab/view/presenter.rb
@@ -0,0 +1,32 @@
+module Gitlab
+  module View
+    module Presenter
+      extend ActiveSupport::Concern
+
+      included do
+        include Gitlab::Routing
+        include Gitlab::Allowable
+      end
+
+      def with_subject(subject)
+        tap { @subject = subject }
+      end
+
+      def with_user(user)
+        tap { @user = user }
+      end
+
+      private
+
+      attr_reader :subject, :user
+
+      class_methods do
+        def presents(name)
+          define_method(name) do
+            subject
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/view/presenter_factory.rb b/lib/gitlab/view/presenter_factory.rb
new file mode 100644
index 00000000000..c8cab1249da
--- /dev/null
+++ b/lib/gitlab/view/presenter_factory.rb
@@ -0,0 +1,39 @@
+module Gitlab
+  module View
+    class PresenterFactory
+      def initialize(subject, user: nil)
+        @subject = subject
+        @user = user
+      end
+
+      def fabricate!
+        presenter =
+          if presenter_class.ancestors.include?(SimpleDelegator)
+            delegator_presenter
+          else
+            simple_presenter
+          end
+
+        presenter
+          .with_subject(subject)
+          .with_user(user)
+      end
+
+      private
+
+      attr_reader :subject, :user
+
+      def presenter_class
+        "#{subject.class.name.demodulize}Presenter".constantize
+      end
+
+      def delegator_presenter
+        presenter_class.new(subject)
+      end
+
+      def simple_presenter
+        presenter_class.new
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/view/presenter_factory_spec.rb b/spec/lib/gitlab/view/presenter_factory_spec.rb
new file mode 100644
index 00000000000..c5e4d86f6c9
--- /dev/null
+++ b/spec/lib/gitlab/view/presenter_factory_spec.rb
@@ -0,0 +1,48 @@
+require 'spec_helper'
+
+describe Gitlab::View::PresenterFactory do
+  let(:appearance) { build(:appearance) }
+  let(:broadcast_message) { build(:broadcast_message) }
+
+  before do
+    class AppearancePresenter
+      include Gitlab::View::Presenter
+    end
+
+    class BroadcastMessagePresenter < SimpleDelegator
+      include Gitlab::View::Presenter
+    end
+  end
+
+  describe '#initialize' do
+    subject do
+      described_class.new(appearance)
+    end
+
+    it 'takes a subject and optional params' do
+      expect { subject }.not_to raise_error
+    end
+  end
+
+  describe '#fabricate!' do
+    context 'without delegation' do
+      subject do
+        described_class.new(appearance).fabricate!
+      end
+
+      it 'does not forward missing methods to subject' do
+        expect { subject.title }.to raise_error(NoMethodError)
+      end
+    end
+
+    context 'with delegation' do
+      subject do
+        described_class.new(broadcast_message).fabricate!
+      end
+
+      it 'forwards missing methods to subject' do
+        expect(subject.message).to eq(broadcast_message.message)
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/view/presenter_spec.rb b/spec/lib/gitlab/view/presenter_spec.rb
new file mode 100644
index 00000000000..0880fbe5d77
--- /dev/null
+++ b/spec/lib/gitlab/view/presenter_spec.rb
@@ -0,0 +1,29 @@
+require 'spec_helper'
+
+describe Gitlab::View::Presenter do
+  let(:project) { double(:project, bar: 'baz!') }
+  let(:presenter) do
+    base_presenter = described_class
+
+    Class.new do
+      include base_presenter
+
+      presents :foo
+    end
+  end
+  subject do
+    presenter.new.with_subject(project)
+  end
+
+  describe '#initialize' do
+    it 'takes an object accessible via a reader' do
+      expect(subject.foo).to eq(project)
+    end
+  end
+
+  describe 'common helpers' do
+    it 'responds to #can?' do
+      expect(subject).to respond_to(:can?)
+    end
+  end
+end
diff --git a/spec/presenters/build_presenter_spec.rb b/spec/presenters/build_presenter_spec.rb
new file mode 100644
index 00000000000..fa7b0567622
--- /dev/null
+++ b/spec/presenters/build_presenter_spec.rb
@@ -0,0 +1,35 @@
+require 'spec_helper'
+
+describe BuildPresenter do
+  let(:build) { create(:ci_build) }
+  subject do
+    described_class.new(build).with_subject(build)
+  end
+
+  describe '#initialize' do
+    it 'takes a build and optional params' do
+      expect { subject }.
+        not_to raise_error
+    end
+
+    it 'exposes build' do
+      expect(subject.build).to eq(build)
+    end
+
+    it 'forwards missing methods to build' do
+      expect(subject.ref).to eq('master')
+    end
+  end
+
+  describe '#erased_by_user?' do
+    it 'takes a build and optional params' do
+      expect(subject).not_to be_erased_by_user
+    end
+  end
+
+  describe 'quack like a Ci::Build' do
+    it 'takes a build and optional params' do
+      expect(described_class.ancestors).to include(Ci::Build)
+    end
+  end
+end
diff --git a/spec/presenters/variable_presenter_spec.rb b/spec/presenters/variable_presenter_spec.rb
new file mode 100644
index 00000000000..f09a0c922ae
--- /dev/null
+++ b/spec/presenters/variable_presenter_spec.rb
@@ -0,0 +1,23 @@
+require 'spec_helper'
+
+describe VariablePresenter do
+  let(:variable) { double(:variable, foo: 'bar') }
+  subject do
+    described_class.new.with_subject(variable)
+  end
+
+  describe '#initialize' do
+    it 'takes a variable and optional params' do
+      expect { subject }.
+        not_to raise_error
+    end
+
+    it 'exposes variable' do
+      expect(subject.variable).to eq(variable)
+    end
+
+    it 'does not forward missing methods to variable' do
+      expect { subject.foo }.to raise_error(NoMethodError)
+    end
+  end
+end