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