Improve presenter architecture

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

app/presenters/build_presenter.rb

-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

app/presenters/ci/build/presenter.rb

+module Ci
+  class Build
+    class Presenter < Gitlab::View::Presenter::Delegated
+      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 erased_by_name
+        erased_by.name if erased_by
+      end
+    end
+  end
+end

app/presenters/ci/variable/presenter.rb

+module Ci
+  class Variable
+    class Presenter < Gitlab::View::Presenter::Simple
+      presents :variable
+    end
+  end
+end

app/presenters/variable_presenter.rb

-class VariablePresenter
-  include Gitlab::View::Presenter
-
-  presents :variable
-end

lib/gitlab/view/presenter.rb → lib/gitlab/view/presenter/base.rb

 module Gitlab
   module View
     module Presenter
-      extend ActiveSupport::Concern
+      module Base
+        extend ActiveSupport::Concern
 
-      included do
         include Gitlab::Routing
         include Gitlab::Allowable
-      end
 
-      def with_subject(subject)
-        tap { @subject = subject }
-      end
+        attr_reader :subject
 
-      def with_user(user)
-        tap { @user = user }
-      end
-
-      private
+        def can?(user, action)
+          super(user, action, subject)
+        end
 
-      attr_reader :subject, :user
+        private
 
-      class_methods do
-        def presents(name)
-          define_method(name) do
-            subject
+        class_methods do
+          def presents(name)
+            define_method(name) { subject }
           end
         end
       end

lib/gitlab/view/presenter/delegated.rb

+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/simple.rb

+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

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

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

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

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

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

spec/lib/gitlab/view/presenter_spec.rb

-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

spec/presenters/build_presenter_spec.rb

-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

spec/presenters/ci/build/presenter_spec.rb

+require 'spec_helper'
+
+describe Ci::Build::Presenter do
+  let(:project) { create(:empty_project) }
+  let(:pipeline) { create(:ci_pipeline, project: project) }
+  let(:build) { create(:ci_build, pipeline: pipeline) }
+
+  subject 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 { 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 '#erased_by_name' do
+    context 'when build is not erased' do
+      before do
+        expect(build).to receive(:erased_by).and_return(nil)
+      end
+
+      it 'returns nil' do
+        expect(subject.erased_by_name).to be_nil
+      end
+    end
+    context 'when build is erased' do
+      before do
+        expect(build).to receive(:erased_by).twice.
+          and_return(double(:user, name: 'John Doe'))
+      end
+
+      it 'returns the name of the eraser' do
+        expect(subject.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) { create(:empty_project, public_builds: false) }
+
+      it 'returns false' do
+        expect(subject.can?(nil, :read_build)).to be_falsy
+      end
+    end
+
+    context 'user is allowed' do
+      let(:project) { create(:empty_project, :public) }
+
+      it 'returns true' do
+        expect(subject.can?(nil, :read_build)).to be_truthy
+      end
+    end
+  end
+end

spec/presenters/variable_presenter_spec.rb → spec/presenters/ci/variable/presenter_spec.rb

 require 'spec_helper'
 
-describe VariablePresenter do
-  let(:variable) { double(:variable, foo: 'bar') }
+describe Ci::Variable::Presenter do
+  let(:variable) { double(:variable) }
+
   subject do
-    described_class.new.with_subject(variable)
+    described_class.new(variable)
+  end
+
+  it 'inherits from Gitlab::View::Presenter::Simple' do
+    expect(described_class.superclass).to eq(Gitlab::View::Presenter::Simple)
   end
 
   describe '#initialize' do
     it 'takes a variable and optional params' do
-      expect { subject }.
-        not_to raise_error
+      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