From 0ac65b6cc31962c293782a1f3e8d6d41922569c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Fri, 20 Jan 2017 18:13:14 +0100 Subject: [PATCH 1/2] Don't override presentee methods for Gitlab::View::Presenter::Delegated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/controllers/projects/builds_controller.rb | 2 +- app/presenters/README.md | 6 +++--- lib/gitlab/view/presenter/delegated.rb | 4 +++- spec/lib/gitlab/view/presenter/delegated_spec.rb | 15 ++++++++++----- spec/lib/gitlab/view/presenter/factory_spec.rb | 7 ------- spec/lib/gitlab/view/presenter/simple_spec.rb | 15 ++++++++++----- 6 files changed, 27 insertions(+), 22 deletions(-) diff --git a/app/controllers/projects/builds_controller.rb b/app/controllers/projects/builds_controller.rb index 9b45ed6b6af..886934a3f67 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(user: current_user) + @build ||= project.builds.find_by!(id: params[:id]).present(current_user: current_user) end def build_path(build) diff --git a/app/presenters/README.md b/app/presenters/README.md index 3edd63451e7..a4d592b54d6 100644 --- a/app/presenters/README.md +++ b/app/presenters/README.md @@ -113,7 +113,7 @@ detects the presenter based on the presented subject's class. class Projects::LabelsController < Projects::ApplicationController def edit @label = Gitlab::View::Presenter::Factory - .new(@label, user: current_user) + .new(@label, current_user: current_user) .fabricate! end end @@ -132,7 +132,7 @@ and then in the controller: ```ruby class Projects::LabelsController < Projects::ApplicationController def edit - @label = @label.present(user: current_user) + @label = @label.present(current_user: current_user) end end ``` @@ -147,7 +147,7 @@ end You can also present the model in the view: ```ruby -- label = @label.present(current_user) +- label = @label.present(current_user: current_user) %div{ class: label.text_color } = render partial: label, label: label diff --git a/lib/gitlab/view/presenter/delegated.rb b/lib/gitlab/view/presenter/delegated.rb index f4d330c590e..fd90f7c2bad 100644 --- a/lib/gitlab/view/presenter/delegated.rb +++ b/lib/gitlab/view/presenter/delegated.rb @@ -8,7 +8,9 @@ module Gitlab @subject = subject attributes.each do |key, value| - define_singleton_method(key) { value } + unless subject.respond_to?(key) + define_singleton_method(key) { value } + end end super(subject) diff --git a/spec/lib/gitlab/view/presenter/delegated_spec.rb b/spec/lib/gitlab/view/presenter/delegated_spec.rb index 888ab80cad5..8390188bfdc 100644 --- a/spec/lib/gitlab/view/presenter/delegated_spec.rb +++ b/spec/lib/gitlab/view/presenter/delegated_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Gitlab::View::Presenter::Delegated do - let(:project) { double(:project, bar: 'baz') } + let(:project) { double(:project, user: 'John Doe') } let(:presenter_class) do Class.new(described_class) end @@ -12,10 +12,15 @@ describe Gitlab::View::Presenter::Delegated do describe '#initialize' do it 'takes arbitrary key/values and exposes them' do - presenter = presenter_class.new(project, user: 'user', foo: 'bar') + presenter = presenter_class.new(project, current_user: 'Jane Doe') - expect(presenter.user).to eq('user') - expect(presenter.foo).to eq('bar') + expect(presenter.current_user).to eq('Jane Doe') + end + + it 'does not override the presentee attributes' do + presenter = presenter_class.new(project, user: 'Jane Doe') + + expect(presenter.user).to eq('John Doe') end end @@ -23,7 +28,7 @@ describe Gitlab::View::Presenter::Delegated do it 'forwards missing methods to subject' do presenter = presenter_class.new(project) - expect(presenter.bar).to eq('baz') + expect(presenter.user).to eq('John Doe') end end end diff --git a/spec/lib/gitlab/view/presenter/factory_spec.rb b/spec/lib/gitlab/view/presenter/factory_spec.rb index 55c5ecbf92f..70d2e22b48f 100644 --- a/spec/lib/gitlab/view/presenter/factory_spec.rb +++ b/spec/lib/gitlab/view/presenter/factory_spec.rb @@ -22,13 +22,6 @@ describe Gitlab::View::Presenter::Factory do end describe '#fabricate!' do - it 'exposes given params' do - 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 presenter = described_class.new(build).fabricate! diff --git a/spec/lib/gitlab/view/presenter/simple_spec.rb b/spec/lib/gitlab/view/presenter/simple_spec.rb index b489bdf1981..1795ed2405b 100644 --- a/spec/lib/gitlab/view/presenter/simple_spec.rb +++ b/spec/lib/gitlab/view/presenter/simple_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Gitlab::View::Presenter::Simple do - let(:project) { double(:project) } + let(:project) { double(:project, user: 'John Doe') } let(:presenter_class) do Class.new(described_class) end @@ -12,10 +12,15 @@ describe Gitlab::View::Presenter::Simple do describe '#initialize' do it 'takes arbitrary key/values and exposes them' do - presenter = presenter_class.new(project, user: 'user', foo: 'bar') + presenter = presenter_class.new(project, current_user: 'Jane Doe') - expect(presenter.user).to eq('user') - expect(presenter.foo).to eq('bar') + expect(presenter.current_user).to eq('Jane Doe') + end + + it 'override the presentee attributes' do + presenter = presenter_class.new(project, user: 'Jane Doe') + + expect(presenter.user).to eq('Jane Doe') end end @@ -23,7 +28,7 @@ describe Gitlab::View::Presenter::Simple do it 'does not forward missing methods to subject' do presenter = presenter_class.new(project) - expect { presenter.foo }.to raise_error(NoMethodError) + expect { presenter.user }.to raise_error(NoMethodError) end end end From 68e94450a06e043ecf58ede565060f41b61043f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 24 Jan 2017 17:07:56 +0100 Subject: [PATCH 2/2] Raise Gitlab::View::Presenter::CannotOverrideMethodError if presentee already respond to method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- lib/gitlab/view/presenter/base.rb | 2 ++ lib/gitlab/view/presenter/delegated.rb | 6 ++++-- spec/lib/gitlab/view/presenter/delegated_spec.rb | 7 +++---- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/lib/gitlab/view/presenter/base.rb b/lib/gitlab/view/presenter/base.rb index 83c8ba5c1cf..dbfe0941e4d 100644 --- a/lib/gitlab/view/presenter/base.rb +++ b/lib/gitlab/view/presenter/base.rb @@ -1,6 +1,8 @@ module Gitlab module View module Presenter + CannotOverrideMethodError = Class.new(StandardError) + module Base extend ActiveSupport::Concern diff --git a/lib/gitlab/view/presenter/delegated.rb b/lib/gitlab/view/presenter/delegated.rb index fd90f7c2bad..387ff0f5d43 100644 --- a/lib/gitlab/view/presenter/delegated.rb +++ b/lib/gitlab/view/presenter/delegated.rb @@ -8,9 +8,11 @@ module Gitlab @subject = subject attributes.each do |key, value| - unless subject.respond_to?(key) - define_singleton_method(key) { value } + if subject.respond_to?(key) + raise CannotOverrideMethodError.new("#{subject} already respond to #{key}!") end + + define_singleton_method(key) { value } end super(subject) diff --git a/spec/lib/gitlab/view/presenter/delegated_spec.rb b/spec/lib/gitlab/view/presenter/delegated_spec.rb index 8390188bfdc..e9d4af54389 100644 --- a/spec/lib/gitlab/view/presenter/delegated_spec.rb +++ b/spec/lib/gitlab/view/presenter/delegated_spec.rb @@ -17,10 +17,9 @@ describe Gitlab::View::Presenter::Delegated do expect(presenter.current_user).to eq('Jane Doe') end - it 'does not override the presentee attributes' do - presenter = presenter_class.new(project, user: 'Jane Doe') - - expect(presenter.user).to eq('John Doe') + it 'raise an error if the presentee already respond to method' do + expect { presenter_class.new(project, user: 'Jane Doe') }. + to raise_error Gitlab::View::Presenter::CannotOverrideMethodError end end