From bf789ff567c71ff68c216bfa8f3d43e09b6f49fb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Mon, 9 Jan 2017 21:45:49 +0100
Subject: [PATCH] Improve presenter architecture
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Rémy Coutable <remy@rymai.me>
---
app/presenters/build_presenter.rb | 15 ----
app/presenters/ci/build/presenter.rb | 17 +++++
app/presenters/ci/variable/presenter.rb | 7 ++
app/presenters/variable_presenter.rb | 5 --
lib/gitlab/view/presenter.rb | 32 --------
lib/gitlab/view/presenter/base.rb | 26 +++++++
lib/gitlab/view/presenter/delegated.rb | 19 +++++
lib/gitlab/view/presenter/simple.rb | 17 +++++
spec/lib/gitlab/view/presenter/base_spec.rb | 38 ++++++++++
.../gitlab/view/presenter/delegated_spec.rb | 33 ++++++++
spec/lib/gitlab/view/presenter/simple_spec.rb | 33 ++++++++
spec/lib/gitlab/view/presenter_spec.rb | 29 -------
spec/presenters/build_presenter_spec.rb | 35 ---------
spec/presenters/ci/build/presenter_spec.rb | 75 +++++++++++++++++++
spec/presenters/ci/variable/presenter_spec.rb | 23 ++++++
spec/presenters/variable_presenter_spec.rb | 23 ------
16 files changed, 288 insertions(+), 139 deletions(-)
delete mode 100644 app/presenters/build_presenter.rb
create mode 100644 app/presenters/ci/build/presenter.rb
create mode 100644 app/presenters/ci/variable/presenter.rb
delete mode 100644 app/presenters/variable_presenter.rb
delete mode 100644 lib/gitlab/view/presenter.rb
create mode 100644 lib/gitlab/view/presenter/base.rb
create mode 100644 lib/gitlab/view/presenter/delegated.rb
create mode 100644 lib/gitlab/view/presenter/simple.rb
create mode 100644 spec/lib/gitlab/view/presenter/base_spec.rb
create mode 100644 spec/lib/gitlab/view/presenter/delegated_spec.rb
create mode 100644 spec/lib/gitlab/view/presenter/simple_spec.rb
delete mode 100644 spec/lib/gitlab/view/presenter_spec.rb
delete mode 100644 spec/presenters/build_presenter_spec.rb
create mode 100644 spec/presenters/ci/build/presenter_spec.rb
create mode 100644 spec/presenters/ci/variable/presenter_spec.rb
delete mode 100644 spec/presenters/variable_presenter_spec.rb
diff --git a/app/presenters/build_presenter.rb b/app/presenters/build_presenter.rb
deleted file mode 100644
index 9c44a6d2dbe..00000000000
--- a/app/presenters/build_presenter.rb
+++ /dev/null
@@ -1,15 +0,0 @@
-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/ci/build/presenter.rb b/app/presenters/ci/build/presenter.rb
new file mode 100644
index 00000000000..60392200fde
--- /dev/null
+++ b/app/presenters/ci/build/presenter.rb
@@ -0,0 +1,17 @@
+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/variable/presenter.rb b/app/presenters/ci/variable/presenter.rb
new file mode 100644
index 00000000000..02045e19cac
--- /dev/null
+++ b/app/presenters/ci/variable/presenter.rb
@@ -0,0 +1,7 @@
+module Ci
+ class Variable
+ class Presenter < Gitlab::View::Presenter::Simple
+ presents :variable
+ end
+ end
+end
diff --git a/app/presenters/variable_presenter.rb b/app/presenters/variable_presenter.rb
deleted file mode 100644
index 80382f3a001..00000000000
--- a/app/presenters/variable_presenter.rb
+++ /dev/null
@@ -1,5 +0,0 @@
-class VariablePresenter
- include Gitlab::View::Presenter
-
- presents :variable
-end
diff --git a/lib/gitlab/view/presenter.rb b/lib/gitlab/view/presenter.rb
deleted file mode 100644
index 8b63b271b99..00000000000
--- a/lib/gitlab/view/presenter.rb
+++ /dev/null
@@ -1,32 +0,0 @@
-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/base.rb b/lib/gitlab/view/presenter/base.rb
new file mode 100644
index 00000000000..51e7ab064fe
--- /dev/null
+++ b/lib/gitlab/view/presenter/base.rb
@@ -0,0 +1,26 @@
+module Gitlab
+ module View
+ module Presenter
+ module Base
+ extend ActiveSupport::Concern
+
+ include Gitlab::Routing
+ include Gitlab::Allowable
+
+ attr_reader :subject
+
+ def can?(user, action)
+ super(user, action, subject)
+ end
+
+ private
+
+ class_methods do
+ def presents(name)
+ define_method(name) { subject }
+ end
+ end
+ end
+ end
+ end
+end
diff --git a/lib/gitlab/view/presenter/delegated.rb b/lib/gitlab/view/presenter/delegated.rb
new file mode 100644
index 00000000000..f4d330c590e
--- /dev/null
+++ b/lib/gitlab/view/presenter/delegated.rb
@@ -0,0 +1,19 @@
+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
diff --git a/lib/gitlab/view/presenter/simple.rb b/lib/gitlab/view/presenter/simple.rb
new file mode 100644
index 00000000000..b7653a0f3cc
--- /dev/null
+++ b/lib/gitlab/view/presenter/simple.rb
@@ -0,0 +1,17 @@
+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
diff --git a/spec/lib/gitlab/view/presenter/base_spec.rb b/spec/lib/gitlab/view/presenter/base_spec.rb
new file mode 100644
index 00000000000..57b98276622
--- /dev/null
+++ b/spec/lib/gitlab/view/presenter/base_spec.rb
@@ -0,0 +1,38 @@
+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
diff --git a/spec/lib/gitlab/view/presenter/delegated_spec.rb b/spec/lib/gitlab/view/presenter/delegated_spec.rb
new file mode 100644
index 00000000000..816d6b7c6d4
--- /dev/null
+++ b/spec/lib/gitlab/view/presenter/delegated_spec.rb
@@ -0,0 +1,33 @@
+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
diff --git a/spec/lib/gitlab/view/presenter/simple_spec.rb b/spec/lib/gitlab/view/presenter/simple_spec.rb
new file mode 100644
index 00000000000..baf074019ec
--- /dev/null
+++ b/spec/lib/gitlab/view/presenter/simple_spec.rb
@@ -0,0 +1,33 @@
+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
diff --git a/spec/lib/gitlab/view/presenter_spec.rb b/spec/lib/gitlab/view/presenter_spec.rb
deleted file mode 100644
index 0880fbe5d77..00000000000
--- a/spec/lib/gitlab/view/presenter_spec.rb
+++ /dev/null
@@ -1,29 +0,0 @@
-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
deleted file mode 100644
index fa7b0567622..00000000000
--- a/spec/presenters/build_presenter_spec.rb
+++ /dev/null
@@ -1,35 +0,0 @@
-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/ci/build/presenter_spec.rb b/spec/presenters/ci/build/presenter_spec.rb
new file mode 100644
index 00000000000..ecab84dcbc9
--- /dev/null
+++ b/spec/presenters/ci/build/presenter_spec.rb
@@ -0,0 +1,75 @@
+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
diff --git a/spec/presenters/ci/variable/presenter_spec.rb b/spec/presenters/ci/variable/presenter_spec.rb
new file mode 100644
index 00000000000..b3afb2e2e33
--- /dev/null
+++ b/spec/presenters/ci/variable/presenter_spec.rb
@@ -0,0 +1,23 @@
+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
diff --git a/spec/presenters/variable_presenter_spec.rb b/spec/presenters/variable_presenter_spec.rb
deleted file mode 100644
index f09a0c922ae..00000000000
--- a/spec/presenters/variable_presenter_spec.rb
+++ /dev/null
@@ -1,23 +0,0 @@
-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
--
GitLab