summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/presenters/README.md165
-rw-r--r--app/presenters/build_presenter.rb15
-rw-r--r--app/presenters/variable_presenter.rb5
-rw-r--r--lib/gitlab/view/presenter.rb32
-rw-r--r--lib/gitlab/view/presenter_factory.rb39
-rw-r--r--spec/lib/gitlab/view/presenter_factory_spec.rb48
-rw-r--r--spec/lib/gitlab/view/presenter_spec.rb29
-rw-r--r--spec/presenters/build_presenter_spec.rb35
-rw-r--r--spec/presenters/variable_presenter_spec.rb23
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
View Lorry Controller Status