More improvements to presenters

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

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]).present(current_user)
+    @build ||= project.builds.find_by!(id: params[:id]).present(user: current_user)
   end
 
   def build_path(build)

app/policies/base_policy.rb

@@ -53,7 +53,7 @@ class BasePolicy
   def self.class_for(subject)
     return GlobalPolicy if subject.nil?
 
-    if subject.class.ancestors.include?(Gitlab::View::Presenter::Base)
+    if subject.class.try(:presenter?)
       subject = subject.subject
     end
 

app/presenters/README.md

@@ -66,7 +66,7 @@ we gain the following benefits:
 
 ### Presenter definition
 
-Every presenters should inherit from `Gitlab::View::Presenter::Simple`, which
+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`.
@@ -76,7 +76,7 @@ class LabelPresenter < Gitlab::View::Presenter::Simple
   presents :label
 
   def text_color
-    LabelsHelper.text_color_for_bg(label.color)
+    label.color.to_s
   end
 
   def to_partial_path
@@ -95,7 +95,7 @@ class LabelPresenter < Gitlab::View::Presenter::Delegated
 
   def text_color
     # color is delegated to label
-    LabelsHelper.text_color_for_bg(color)
+    color.to_s
   end
 
   def to_partial_path
@@ -132,7 +132,7 @@ and then in the controller:
 ```ruby
 class Projects::LabelsController < Projects::ApplicationController
   def edit
-    @label = @label.present(current_user)
+    @label = @label.present(user: current_user)
   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/build_presenter.rb

+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/presenters/ci/variable/presenter.rb

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

lib/gitlab/view/presenter/base.rb

@@ -9,13 +9,15 @@ module Gitlab
 
         attr_reader :subject
 
-        def can?(user, action)
-          super(user, action, subject)
+        def can?(user, action, overriden_subject = nil)
+          super(user, action, overriden_subject || subject)
         end
 
-        private
-
         class_methods do
+          def presenter?
+            true
+          end
+
           def presents(name)
             define_method(name) { subject }
           end

lib/gitlab/view/presenter/factory.rb

@@ -8,13 +8,15 @@ module Gitlab
         end
 
         def fabricate!
-          presenter_class.new(@subject, @attributes)
+          presenter_class.new(subject, attributes)
         end
 
         private
 
+        attr_reader :subject, :attributes
+
         def presenter_class
-          @subject.class.const_get('Presenter')
+          "#{subject.class.name}Presenter".constantize
         end
       end
     end

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

@@ -6,32 +6,45 @@ describe Gitlab::View::Presenter::Base do
     Struct.new(:subject).include(described_class)
   end
 
-  subject do
-    presenter_class.new(project)
+  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(subject.foo).to eq(project)
+      expect(presenter.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
+        presenter = presenter_class.new(build_stubbed(:empty_project))
+
+        expect(presenter.can?(nil, :read_project)).to be_falsy
       end
     end
 
     context 'user is allowed' do
-      let(:project) { create(:empty_project, :public) }
+      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
-        expect(subject.can?(nil, :read_project)).to be_truthy
+        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

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

 require 'spec_helper'
 
 describe Gitlab::View::Presenter::Delegated do
-  let(:project) { double(:project, foo: 'bar') }
+  let(:project) { double(:project, bar: 'baz') }
   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')
+      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
-      expect(subject.foo).to eq('bar')
+    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

 require 'spec_helper'
 
 describe Gitlab::View::Presenter::Factory do
-  let(:variable) { create(:ci_variable) }
+  let(:build) { Ci::Build.new }
 
   describe '#initialize' do
     context 'without optional parameters' do
-      subject do
-        described_class.new(variable)
-      end
-
       it 'takes a subject and optional params' do
-        expect { subject }.not_to raise_error
+        presenter = described_class.new(build)
+
+        expect { presenter }.not_to raise_error
       end
     end
 
     context 'with optional parameters' do
-      subject do
-        described_class.new(variable, user: 'user')
-      end
-
       it 'takes a subject and optional params' do
-        expect { subject }.not_to raise_error
+        presenter = described_class.new(build, user: 'user')
+
+        expect { presenter }.not_to raise_error
       end
     end
   end
 
   describe '#fabricate!' do
-    subject do
-      described_class.new(variable, user: 'user', foo: 'bar').fabricate!
-    end
-
     it 'exposes given params' do
-      expect(subject.user).to eq('user')
-      expect(subject.foo).to eq('bar')
+      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
-      expect(subject).to be_a(Ci::Variable::Presenter)
+      presenter = described_class.new(build).fabricate!
+
+      expect(presenter).to be_a(Ci::BuildPresenter)
     end
   end
 end

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

@@ -6,28 +6,24 @@ describe Gitlab::View::Presenter::Simple 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')
+      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
-      expect { subject.foo }.to raise_error(NoMethodError)
+      presenter = presenter_class.new(project)
+
+      expect { presenter.foo }.to raise_error(NoMethodError)
     end
   end
 end

spec/models/concerns/presentable_spec.rb

 require 'spec_helper'
 
 describe Presentable do
-  let(:build) { create(:ci_build) }
+  let(:build) { Ci::Build.new }
 
   describe '#present' do
     it 'returns a presenter' do
-      expect(build.present).to be_a(Ci::Build::Presenter)
+      expect(build.present).to be_a(Ci::BuildPresenter)
     end
 
     it 'takes optional attributes' do

spec/policies/base_policy_spec.rb

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

spec/presenters/ci/build/presenter_spec.rb → spec/presenters/ci/build_presenter_spec.rb

 require 'spec_helper'
 
-describe Ci::Build::Presenter do
+describe Ci::BuildPresenter do
   let(:project) { create(:empty_project) }
   let(:pipeline) { create(:ci_pipeline, project: project) }
   let(:build) { create(:ci_build, pipeline: pipeline) }
 
-  subject do
+  subject(:presenter) do
     described_class.new(build)
   end
 
@@ -15,60 +15,62 @@ describe Ci::Build::Presenter do
 
   describe '#initialize' do
     it 'takes a build and optional params' do
-      expect { subject }.not_to raise_error
+      expect { presenter }.not_to raise_error
     end
 
     it 'exposes build' do
-      expect(subject.build).to eq(build)
+      expect(presenter.build).to eq(build)
     end
 
     it 'forwards missing methods to build' do
-      expect(subject.ref).to eq('master')
+      expect(presenter.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
+      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(build).to receive(:erased_by).and_return(nil)
+        expect(presenter).to receive(:erased_by_user?).and_return(false)
       end
 
       it 'returns nil' do
-        expect(subject.erased_by_name).to be_nil
+        expect(presenter.erased_by_name).to be_nil
       end
     end
+
     context 'when build is erased' do
       before do
-        expect(build).to receive(:erased_by).twice.
+        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(subject.erased_by_name).to eq('John Doe')
+        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) { create(:empty_project, public_builds: false) }
+      let(:project) { build_stubbed(:empty_project, public_builds: false) }
 
       it 'returns false' do
-        expect(subject.can?(nil, :read_build)).to be_falsy
+        expect(presenter.can?(nil, :read_build)).to be_falsy
       end
     end
 
     context 'user is allowed' do
-      let(:project) { create(:empty_project, :public) }
+      let(:project) { build_stubbed(:empty_project, :public) }
 
       it 'returns true' do
-        expect(subject.can?(nil, :read_build)).to be_truthy
+        expect(presenter.can?(nil, :read_build)).to be_truthy
       end
     end
   end

spec/presenters/ci/variable/presenter_spec.rb

-require 'spec_helper'
-
-describe Ci::Variable::Presenter do
-  let(:variable) { double(:variable) }
-
-  subject do
-    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
-    end
-
-    it 'exposes variable' do
-      expect(subject.variable).to eq(variable)
-    end
-  end
-end