diff --git a/app/controllers/projects/builds_controller.rb b/app/controllers/projects/builds_controller.rb index 0096ea77a12..9b45ed6b6af 100644 --- a/app/controllers/projects/builds_controller.rb +++ b/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) diff --git a/app/policies/base_policy.rb b/app/policies/base_policy.rb index 43b4a15b81a..b9f1c29c32e 100644 --- a/app/policies/base_policy.rb +++ b/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 diff --git a/app/presenters/README.md b/app/presenters/README.md index 91c1d2609f5..3edd63451e7 100644 --- a/app/presenters/README.md +++ b/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 ``` diff --git a/app/presenters/ci/build/presenter.rb b/app/presenters/ci/build/presenter.rb deleted file mode 100644 index 60392200fde..00000000000 --- a/app/presenters/ci/build/presenter.rb +++ /dev/null @@ -1,17 +0,0 @@ -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 diff --git a/app/presenters/ci/build_presenter.rb b/app/presenters/ci/build_presenter.rb new file mode 100644 index 00000000000..ed72ed14d72 --- /dev/null +++ b/app/presenters/ci/build_presenter.rb @@ -0,0 +1,15 @@ +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 diff --git a/app/presenters/ci/variable/presenter.rb b/app/presenters/ci/variable/presenter.rb deleted file mode 100644 index 02045e19cac..00000000000 --- a/app/presenters/ci/variable/presenter.rb +++ /dev/null @@ -1,7 +0,0 @@ -module Ci - class Variable - class Presenter < Gitlab::View::Presenter::Simple - presents :variable - end - end -end diff --git a/lib/gitlab/view/presenter/base.rb b/lib/gitlab/view/presenter/base.rb index 51e7ab064fe..83c8ba5c1cf 100644 --- a/lib/gitlab/view/presenter/base.rb +++ b/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 diff --git a/lib/gitlab/view/presenter/factory.rb b/lib/gitlab/view/presenter/factory.rb index 92979c61a25..d172d61e2c9 100644 --- a/lib/gitlab/view/presenter/factory.rb +++ b/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 diff --git a/spec/lib/gitlab/view/presenter/base_spec.rb b/spec/lib/gitlab/view/presenter/base_spec.rb index 57b98276622..f2c152cdcd4 100644 --- a/spec/lib/gitlab/view/presenter/base_spec.rb +++ b/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 - expect(subject.can?(nil, :read_project)).to be_truthy + 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 + 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 diff --git a/spec/lib/gitlab/view/presenter/delegated_spec.rb b/spec/lib/gitlab/view/presenter/delegated_spec.rb index 816d6b7c6d4..888ab80cad5 100644 --- a/spec/lib/gitlab/view/presenter/delegated_spec.rb +++ b/spec/lib/gitlab/view/presenter/delegated_spec.rb @@ -1,33 +1,29 @@ 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 diff --git a/spec/lib/gitlab/view/presenter/factory_spec.rb b/spec/lib/gitlab/view/presenter/factory_spec.rb index 7a65429b500..55c5ecbf92f 100644 --- a/spec/lib/gitlab/view/presenter/factory_spec.rb +++ b/spec/lib/gitlab/view/presenter/factory_spec.rb @@ -1,42 +1,38 @@ 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 diff --git a/spec/lib/gitlab/view/presenter/simple_spec.rb b/spec/lib/gitlab/view/presenter/simple_spec.rb index baf074019ec..b489bdf1981 100644 --- a/spec/lib/gitlab/view/presenter/simple_spec.rb +++ b/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 diff --git a/spec/models/concerns/presentable_spec.rb b/spec/models/concerns/presentable_spec.rb index 6640a5e1377..941647a79fb 100644 --- a/spec/models/concerns/presentable_spec.rb +++ b/spec/models/concerns/presentable_spec.rb @@ -1,11 +1,11 @@ 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 diff --git a/spec/policies/base_policy_spec.rb b/spec/policies/base_policy_spec.rb index 439d6436f34..63acc0b68cd 100644 --- a/spec/policies/base_policy_spec.rb +++ b/spec/policies/base_policy_spec.rb @@ -1,7 +1,7 @@ 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 diff --git a/spec/presenters/ci/build/presenter_spec.rb b/spec/presenters/ci/build_presenter_spec.rb similarity index 60% rename from spec/presenters/ci/build/presenter_spec.rb rename to spec/presenters/ci/build_presenter_spec.rb index ecab84dcbc9..7a35da38b2b 100644 --- a/spec/presenters/ci/build/presenter_spec.rb +++ b/spec/presenters/ci/build_presenter_spec.rb @@ -1,11 +1,11 @@ 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 diff --git a/spec/presenters/ci/variable/presenter_spec.rb b/spec/presenters/ci/variable/presenter_spec.rb deleted file mode 100644 index b3afb2e2e33..00000000000 --- a/spec/presenters/ci/variable/presenter_spec.rb +++ /dev/null @@ -1,23 +0,0 @@ -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