From 5752cd89728aa4f200939366f7b6644d7ecc308e Mon Sep 17 00:00:00 2001 From: Saverio Trioni Date: Wed, 9 May 2018 00:24:02 +0200 Subject: [PATCH 01/12] Use membership decorator in view [WIP] --- Gemfile | 1 + Gemfile.lock | 2 + app/decorators/member_decorator.rb | 54 +++++++++++++++++ app/views/users/_user_rows.html.erb | 38 ++++-------- spec/decorators/member_decorator_spec.rb | 77 ++++++++++++++++++++++++ 5 files changed, 147 insertions(+), 25 deletions(-) create mode 100644 app/decorators/member_decorator.rb create mode 100644 spec/decorators/member_decorator_spec.rb diff --git a/Gemfile b/Gemfile index 88aec66df..32c8bda62 100644 --- a/Gemfile +++ b/Gemfile @@ -6,6 +6,7 @@ gem 'rails', '~> 4.2' gem 'rails-i18n' gem "rdiscount" gem 'activeadmin', '~> 1.2.1' +gem 'dry-initializer' gem 'has_scope' gem 'pundit' gem 'pg', '0.17.1' diff --git a/Gemfile.lock b/Gemfile.lock index fb5b77c75..66dba6411 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -117,6 +117,7 @@ GEM dotenv (1.0.2) dotenv-rails (1.0.2) dotenv (= 1.0.2) + dry-initializer (2.4.0) elasticsearch (1.0.8) elasticsearch-api (= 1.0.7) elasticsearch-transport (= 1.0.7) @@ -381,6 +382,7 @@ DEPENDENCIES database_cleaner (= 1.3.0) devise (~> 4.4.1) dotenv-rails (= 1.0.2) + dry-initializer elasticsearch-model elasticsearch-rails fabrication diff --git a/app/decorators/member_decorator.rb b/app/decorators/member_decorator.rb new file mode 100644 index 000000000..cb6f30890 --- /dev/null +++ b/app/decorators/member_decorator.rb @@ -0,0 +1,54 @@ +require 'dry-initializer' + +class MemberDecorator + extend Dry::Initializer + + param :member + param :view_context + + alias_method :v, :view_context + + delegate :user, :member_uid, :active?, to: :member + delegate :phone, :alt_phone, :username, to: :user + + def row_css_class + 'bg-danger' unless active? + end + + def inactive_icon + v.tag(:span, class: %w[glyphicon glyphicon-time]) unless active? + end + + def link_to_self + v.link_to(user.username, v.user_path(user)) + end + + def mail_to + email = user.unconfirmed_email || user.email + v.mail_to(email) if email && !email.end_with?('example.com') + end + + def avatar_img + v.image_tag(v.avatar_url(user, 32), width: 32, height: 32) + end + + def account_balance + v.seconds_to_hm(membership.account.try(:balance) || 0) + end + + def edit_user_path + v.edit_user_path(user) + end + + def toggle_manager_member_path + v.toggle_manager_member_path(member) + end + + def cancel_member_path + v.cancel_member_path(member) + end + + def toggle_active_member_path + v.toggle_active_member_path(member) + end +end diff --git a/app/views/users/_user_rows.html.erb b/app/views/users/_user_rows.html.erb index 3bb148ec4..7ca12da6c 100644 --- a/app/views/users/_user_rows.html.erb +++ b/app/views/users/_user_rows.html.erb @@ -1,38 +1,26 @@ <% users.each do |user| %> <% membership = memberships[user.id] %> - <%= content_tag(:tr, class: membership.active? ? "" : "bg-danger") do %> + <%= content_tag(:tr, class: membership.row_css_class) do %> + <%= membership.avatar %> + <%= membership.member_uid %> - <%= image_tag avatar_url(user, 32), width: 32, height: 32 %> + <%= membership.inactive_icon %> + <%= membership.link_to_self %> - <%= membership.member_uid %> - - <% if !membership.active? %><% end %> - <%= link_to user.username, user_path(user) %> - - - <% if user.unconfirmed_email %> - - <%= user.unconfirmed_email %> - - <% else %> - - <%= user.email %> - - <% end %> - - <%= user.phone %> - <%= user.alt_phone %> - <%= seconds_to_hm(membership.account.try(:balance) || mdash) %> + <%= membership.mail_to %> + <%= membership.phone %> + <%= membership.alt_phone %> + <%= membership.account_balance %> <% if current_user.manages?(current_organization) %> - <%= link_to edit_user_path(user), class: "action" do %> + <%= link_to membership.edit_user_path, class: "action" do %> <%= glyph :pencil %> <%= t "global.edit" %> <% end %> <% if membership.active? %> - <%= link_to toggle_manager_member_path(membership), class: "action", method: :put, data: { confirm: t('users.index.manage_warning', username: user.username) } do %> + <%= link_to membership.toggle_manager_member_path, class: "action", method: :put, data: { confirm: t('users.index.manage_warning', username: membership.username) } do %> <% if !!membership.manager %> <%= glyph :arrow_down %> <%= t "global.demote" %> @@ -42,13 +30,13 @@ <% end %> <% end %> <% else %> - <%= link_to cancel_member_path(membership), class: "action", data: { confirm: t('.cancel_warning', user: user.username) }, method: :delete do %> + <%= link_to membership.cancel_member_path, class: "action", data: { confirm: t('.cancel_warning', user: membership.username) }, method: :delete do %> <%= glyph :ban_circle %> <%= t("global.cancel_membership") %> <% end %> <% end %> - <%= link_to toggle_active_member_path(membership), class: "action", method: :put, data: { confirm: t('users.index.active_warning', username: user.username) } do %> + <%= link_to membership.toggle_active_member_path, class: "action", method: :put, data: { confirm: t('users.index.active_warning', username: membership.username) } do %> <% if membership.active? %> <%= glyph :remove %> <%= t ".deactivate" %> diff --git a/spec/decorators/member_decorator_spec.rb b/spec/decorators/member_decorator_spec.rb new file mode 100644 index 000000000..7db760d1c --- /dev/null +++ b/spec/decorators/member_decorator_spec.rb @@ -0,0 +1,77 @@ +require "spec_helper" + +describe MemberDecorator do + let(:org) { Fabricate(:organization) } + let(:member) { Fabricate(:member, organization: org) } + let(:view_context) { ApplicationController.new.view_context } + let(:decorator) { MemberDecorator.new(member, view_context) } + + describe '#row_css_class' do + subject { decorator.row_css_class } + end + + describe '#inactive_icon' do + subject { decorator.inactive_icon } + end + + describe '#link_to_self' do + subject { decorator.link_to_self } + end + + describe '#mail_to' do + subject { decorator.mail_to } + + context 'with standard email' do + let(:email) { 'foobar@gmail.com' } + + context 'unconfirmed' do + before { member.user.update_attributes(unconfirmed_email: email) } + it { is_expected.to include('mailto:foobar@gmail.com') } + end + + context 'confirmed' do + before { member.user.update_attributes(email: email) } + it { is_expected.to include('mailto:foobar@gmail.com') } + end + end + + context 'with placeholder email' do + let(:email) { 'foobar@example.com' } + + context 'unconfirmed' do + before { member.user.update_attributes(unconfirmed_email: email) } + it { is_expected.to be_nil } + end + + context 'confirmed' do + before { member.user.update_attributes(email: email) } + it { is_expected.to be_nil } + end + end + end + + describe '#avatar_img' do + subject { decorator.avatar_img } + it { is_expected.to match(/gravatar/)} + end + + describe '#account_balance' do + subject { decorator.account_balance } + end + + describe '#edit_user_path' do + subject { decorator.edit_user_path } + end + + describe '#toggle_manager_member_path' do + subject { decorator.toggle_manager_member_path } + end + + describe '#cancel_member_path' do + subject { decorator.cancel_member_path } + end + + describe '#toggle_active_member_path' do + subject { decorator.toggle_active_member_path } + end +end \ No newline at end of file From f6e512805f040ddd2bb49b39e8fbd70107c5572c Mon Sep 17 00:00:00 2001 From: Saverio Trioni Date: Wed, 9 May 2018 00:48:35 +0200 Subject: [PATCH 02/12] WIP --- app/controllers/users_controller.rb | 7 +++-- app/views/users/_user_rows.html.erb | 38 +++++++++++------------- spec/decorators/member_decorator_spec.rb | 18 +++++++++++ 3 files changed, 40 insertions(+), 23 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index b56fba5c2..c951a5eed 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -15,9 +15,10 @@ def index @memberships = current_organization.members. where(user_id: @users.map(&:id)). - includes(:account).each_with_object({}) do |mem, ob| - ob[mem.user_id] = mem - end + includes(:account, :user). + to_a. + sort_by { |m| @users.index(m.user) }. + map { |m| MemberDecorator.new(m, view_context) } end def show diff --git a/app/views/users/_user_rows.html.erb b/app/views/users/_user_rows.html.erb index 7ca12da6c..29af00b52 100644 --- a/app/views/users/_user_rows.html.erb +++ b/app/views/users/_user_rows.html.erb @@ -1,27 +1,25 @@ -<% users.each do |user| %> - <% membership = memberships[user.id] %> - - <%= content_tag(:tr, class: membership.row_css_class) do %> - <%= membership.avatar %> - <%= membership.member_uid %> +<% members.each do |member| %> + <%= content_tag(:tr, class: member.row_css_class) do %> + <%= member.avatar %> + <%= member.member_uid %> - <%= membership.inactive_icon %> - <%= membership.link_to_self %> + <%= member.inactive_icon %> + <%= member.link_to_self %> - <%= membership.mail_to %> - <%= membership.phone %> - <%= membership.alt_phone %> - <%= membership.account_balance %> + <%= member.mail_to %> + <%= member.phone %> + <%= member.alt_phone %> + <%= member.account_balance %> <% if current_user.manages?(current_organization) %> - <%= link_to membership.edit_user_path, class: "action" do %> + <%= link_to member.edit_user_path, class: "action" do %> <%= glyph :pencil %> <%= t "global.edit" %> <% end %> - <% if membership.active? %> - <%= link_to membership.toggle_manager_member_path, class: "action", method: :put, data: { confirm: t('users.index.manage_warning', username: membership.username) } do %> - <% if !!membership.manager %> + <% if member.active? %> + <%= link_to member.toggle_manager_member_path, class: "action", method: :put, data: { confirm: t('users.index.manage_warning', username: member.username) } do %> + <% if !!member.manager %> <%= glyph :arrow_down %> <%= t "global.demote" %> <% else %> @@ -30,14 +28,14 @@ <% end %> <% end %> <% else %> - <%= link_to membership.cancel_member_path, class: "action", data: { confirm: t('.cancel_warning', user: membership.username) }, method: :delete do %> + <%= link_to member.cancel_member_path, class: "action", data: { confirm: t('.cancel_warning', user: member.username) }, method: :delete do %> <%= glyph :ban_circle %> - <%= t("global.cancel_membership") %> + <%= t("global.cancel_member") %> <% end %> <% end %> - <%= link_to membership.toggle_active_member_path, class: "action", method: :put, data: { confirm: t('users.index.active_warning', username: membership.username) } do %> - <% if membership.active? %> + <%= link_to member.toggle_active_member_path, class: "action", method: :put, data: { confirm: t('users.index.active_warning', username: member.username) } do %> + <% if member.active? %> <%= glyph :remove %> <%= t ".deactivate" %> <% else %> diff --git a/spec/decorators/member_decorator_spec.rb b/spec/decorators/member_decorator_spec.rb index 7db760d1c..305d6efad 100644 --- a/spec/decorators/member_decorator_spec.rb +++ b/spec/decorators/member_decorator_spec.rb @@ -8,10 +8,28 @@ describe '#row_css_class' do subject { decorator.row_css_class } + + context 'active member' do + it { is_expected.to be_nil } + end + + context 'inactive member' do + before { member.update_attributes(active: false) } + it { is_expected.to eq('bg-danger') } + end end describe '#inactive_icon' do subject { decorator.inactive_icon } + + context 'active member' do + it { is_expected.to be_nil } + end + + context 'inactive member' do + before { member.update_attributes(active: false) } + it { is_expected.to match('icon') } + end end describe '#link_to_self' do From 5c2c25b57af000307ec62e694a16ad76f1842fe1 Mon Sep 17 00:00:00 2001 From: Saverio Trioni Date: Wed, 9 May 2018 00:53:08 +0200 Subject: [PATCH 03/12] WIP --- app/controllers/users_controller.rb | 2 +- app/views/users/index.html.erb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index c951a5eed..c068602e3 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -13,7 +13,7 @@ def index .page(params[:page]) .per(25) - @memberships = current_organization.members. + @members = current_organization.members. where(user_id: @users.map(&:id)). includes(:account, :user). to_a. diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index fea03f0d9..6a43807db 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -54,7 +54,7 @@ - <%= render "user_rows", users: @users, memberships: @memberships %> + <%= render "user_rows", members: @members %> From 67f84238e19307165c76e5d621376c13d191bb84 Mon Sep 17 00:00:00 2001 From: Saverio Trioni Date: Wed, 9 May 2018 02:38:52 +0200 Subject: [PATCH 04/12] Added missing tests; extract partials from row --- app/decorators/member_decorator.rb | 22 ++++++++++----- .../users/_cancel_membership_link.html.erb | 7 +++++ app/views/users/_toggle_active_link.html.erb | 12 +++++++++ app/views/users/_toggle_manager_link.html.erb | 12 +++++++++ app/views/users/_user_rows.html.erb | 27 +++---------------- spec/controllers/users_controller_spec.rb | 14 +++++----- spec/decorators/member_decorator_spec.rb | 18 ++++++++++++- 7 files changed, 75 insertions(+), 37 deletions(-) create mode 100644 app/views/users/_cancel_membership_link.html.erb create mode 100644 app/views/users/_toggle_active_link.html.erb create mode 100644 app/views/users/_toggle_manager_link.html.erb diff --git a/app/decorators/member_decorator.rb b/app/decorators/member_decorator.rb index cb6f30890..7b5a79ece 100644 --- a/app/decorators/member_decorator.rb +++ b/app/decorators/member_decorator.rb @@ -11,6 +11,10 @@ class MemberDecorator delegate :user, :member_uid, :active?, to: :member delegate :phone, :alt_phone, :username, to: :user + def manager? + !!member.manager + end + def row_css_class 'bg-danger' unless active? end @@ -20,7 +24,7 @@ def inactive_icon end def link_to_self - v.link_to(user.username, v.user_path(user)) + v.link_to(user.username, h.user_path(user)) end def mail_to @@ -33,22 +37,28 @@ def avatar_img end def account_balance - v.seconds_to_hm(membership.account.try(:balance) || 0) + v.seconds_to_hm(member.account.try(:balance) || 0) end def edit_user_path - v.edit_user_path(user) + h.edit_user_path(user) end def toggle_manager_member_path - v.toggle_manager_member_path(member) + h.toggle_manager_member_path(member) end def cancel_member_path - v.cancel_member_path(member) + h.member_path(member) end def toggle_active_member_path - v.toggle_active_member_path(member) + h.toggle_active_member_path(member) + end + + private + + def h + Rails.application.routes.url_helpers end end diff --git a/app/views/users/_cancel_membership_link.html.erb b/app/views/users/_cancel_membership_link.html.erb new file mode 100644 index 000000000..393bfb567 --- /dev/null +++ b/app/views/users/_cancel_membership_link.html.erb @@ -0,0 +1,7 @@ +<%= link_to member.cancel_member_path, + class: 'action', + method: :delete, + data: { confirm: t('users.user_rows.cancel_warning', user: member.username) } do %> + <%= glyph :ban_circle %> + <%= t('global.cancel_member') %> +<% end %> \ No newline at end of file diff --git a/app/views/users/_toggle_active_link.html.erb b/app/views/users/_toggle_active_link.html.erb new file mode 100644 index 000000000..9215d48e4 --- /dev/null +++ b/app/views/users/_toggle_active_link.html.erb @@ -0,0 +1,12 @@ +<%= link_to member.toggle_active_member_path, + class: 'action', + method: :put, + data: { confirm: t('users.index.active_warning', username: member.username) } do %> + <% if member.active? %> + <%= glyph :remove %> + <%= t 'users.user_rows.deactivate' %> + <% else %> + <%= glyph :ok %> + <%= t 'users.user_rows.activate' %> + <% end %> +<% end %> diff --git a/app/views/users/_toggle_manager_link.html.erb b/app/views/users/_toggle_manager_link.html.erb new file mode 100644 index 000000000..52694b977 --- /dev/null +++ b/app/views/users/_toggle_manager_link.html.erb @@ -0,0 +1,12 @@ +<%= link_to member.toggle_manager_member_path, + class: 'action', + method: :put, + data: { confirm: t('users.index.manage_warning', username: member.username) } do %> + <% if member.manager? %> + <%= glyph :arrow_down %> + <%= t 'global.demote' %> + <% else %> + <%= glyph :arrow_up %> + <%= t 'global.promote' %> + <% end %> +<% end %> \ No newline at end of file diff --git a/app/views/users/_user_rows.html.erb b/app/views/users/_user_rows.html.erb index 29af00b52..6d53a9f6f 100644 --- a/app/views/users/_user_rows.html.erb +++ b/app/views/users/_user_rows.html.erb @@ -1,6 +1,6 @@ <% members.each do |member| %> <%= content_tag(:tr, class: member.row_css_class) do %> - <%= member.avatar %> + <%= member.avatar_img %> <%= member.member_uid %> <%= member.inactive_icon %> @@ -18,31 +18,12 @@ <% end %> <% if member.active? %> - <%= link_to member.toggle_manager_member_path, class: "action", method: :put, data: { confirm: t('users.index.manage_warning', username: member.username) } do %> - <% if !!member.manager %> - <%= glyph :arrow_down %> - <%= t "global.demote" %> - <% else %> - <%= glyph :arrow_up %> - <%= t "global.promote" %> - <% end %> - <% end %> + <%= render 'toggle_manager_link', member: member if can_toggle_manager?(member) %> <% else %> - <%= link_to member.cancel_member_path, class: "action", data: { confirm: t('.cancel_warning', user: member.username) }, method: :delete do %> - <%= glyph :ban_circle %> - <%= t("global.cancel_member") %> - <% end %> + <%= render 'cancel_membership_link', member: member if can_cancel_member?(member) %> <% end %> - <%= link_to member.toggle_active_member_path, class: "action", method: :put, data: { confirm: t('users.index.active_warning', username: member.username) } do %> - <% if member.active? %> - <%= glyph :remove %> - <%= t ".deactivate" %> - <% else %> - <%= glyph :ok %> - <%= t ".activate" %> - <% end %> - <% end %> + <%= render 'toggle_active_link', member: member if can_toggle_active?(member) %> <% end %> <% end %> diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 9f5df70e2..e27893b22 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -67,13 +67,13 @@ it 'gets her membership in the current organization' do get :index - expect(assigns(:memberships)).to eq({ - member.user_id => member, - another_member.user_id => another_member, - member_admin.user_id => member_admin, - wrong_email_member.user_id => wrong_email_member, - empty_email_member.user_id => empty_email_member - }) + expect(assigns(:members)).to eq([ + member, + another_member, + member_admin, + wrong_email_member, + empty_email_member + ]) end it 'shows data for her membership in the current organization' do diff --git a/spec/decorators/member_decorator_spec.rb b/spec/decorators/member_decorator_spec.rb index 305d6efad..ac5ea1519 100644 --- a/spec/decorators/member_decorator_spec.rb +++ b/spec/decorators/member_decorator_spec.rb @@ -3,7 +3,7 @@ describe MemberDecorator do let(:org) { Fabricate(:organization) } let(:member) { Fabricate(:member, organization: org) } - let(:view_context) { ApplicationController.new.view_context } + let(:view_context) { ApplicationController.new.view_context.tap { |ctx| ctx.singleton_class.include Rails.application.routes.url_helpers } } let(:decorator) { MemberDecorator.new(member, view_context) } describe '#row_css_class' do @@ -34,6 +34,7 @@ describe '#link_to_self' do subject { decorator.link_to_self } + it { is_expected.to match("members/#{member.user.id}")} end describe '#mail_to' do @@ -75,21 +76,36 @@ describe '#account_balance' do subject { decorator.account_balance } + it { is_expected.to eq('—') } + + context 'with positive balance' do + before { member.account.update_attribute(:balance, 3600) } + it { is_expected.to eq('1:00') } + end + + context 'with negative balance' do + before { member.account.update_attribute(:balance, -7500) } + it { is_expected.to eq('-2:05') } + end end describe '#edit_user_path' do subject { decorator.edit_user_path } + it { is_expected.to include("members/#{member.user.id}/edit")} end describe '#toggle_manager_member_path' do subject { decorator.toggle_manager_member_path } + it { is_expected.to include("members/#{member.user.id}/toggle_manager")} end describe '#cancel_member_path' do subject { decorator.cancel_member_path } + it { is_expected.to include("members/#{member.user.id}")} end describe '#toggle_active_member_path' do subject { decorator.toggle_active_member_path } + it { is_expected.to include("members/#{member.user.id}/toggle_active")} end end \ No newline at end of file From 8b7595caeeb461bced2ef7a850fda99bfe3cacf8 Mon Sep 17 00:00:00 2001 From: Saverio Trioni Date: Wed, 9 May 2018 03:20:23 +0200 Subject: [PATCH 05/12] Fix last test --- app/controllers/users_controller.rb | 23 ++++++++++++++++++----- spec/controllers/users_controller_spec.rb | 4 +++- spec/decorators/member_decorator_spec.rb | 4 ++-- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index c068602e3..0f806f399 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -13,12 +13,25 @@ def index .page(params[:page]) .per(25) - @members = current_organization.members. - where(user_id: @users.map(&:id)). + user_ids = @users.pluck(:id) + + @members = current_organization. + members. + where(user_id: user_ids). includes(:account, :user). - to_a. - sort_by { |m| @users.index(m.user) }. - map { |m| MemberDecorator.new(m, view_context) } + each_with_object({}) { |mem, ob| ob[mem.user_id] = mem }. + values_at(*user_ids). + map { |m| MemberDecorator.new(m, self.class.helpers) } + + # TODO: mutate theparameters so they can be used directly on the + # members table. + # + # @members = current_organization.members. + # joins(:account, :user). + # merge(@search.result(distinct: false)). + # page(params[:page]). + # per(25). + # map { |m| MemberDecorator.new(m, self.class.helpers) } end def show diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index e27893b22..6f87e844c 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -67,7 +67,9 @@ it 'gets her membership in the current organization' do get :index - expect(assigns(:members)).to eq([ + assigns(:members).each { |m| expect(m).to be_a(MemberDecorator) } + + expect(assigns(:members).map(&:member)).to eq([ member, another_member, member_admin, diff --git a/spec/decorators/member_decorator_spec.rb b/spec/decorators/member_decorator_spec.rb index ac5ea1519..b028299f8 100644 --- a/spec/decorators/member_decorator_spec.rb +++ b/spec/decorators/member_decorator_spec.rb @@ -3,7 +3,7 @@ describe MemberDecorator do let(:org) { Fabricate(:organization) } let(:member) { Fabricate(:member, organization: org) } - let(:view_context) { ApplicationController.new.view_context.tap { |ctx| ctx.singleton_class.include Rails.application.routes.url_helpers } } + let(:view_context) { ApplicationController.new.view_context } let(:decorator) { MemberDecorator.new(member, view_context) } describe '#row_css_class' do @@ -108,4 +108,4 @@ subject { decorator.toggle_active_member_path } it { is_expected.to include("members/#{member.user.id}/toggle_active")} end -end \ No newline at end of file +end From acc0a61c16b7a6ed9014510138678acf32d27c8b Mon Sep 17 00:00:00 2001 From: Saverio Trioni Date: Wed, 9 May 2018 03:23:24 +0200 Subject: [PATCH 06/12] Fix tests after running whole suite --- spec/decorators/member_decorator_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/decorators/member_decorator_spec.rb b/spec/decorators/member_decorator_spec.rb index b028299f8..006596390 100644 --- a/spec/decorators/member_decorator_spec.rb +++ b/spec/decorators/member_decorator_spec.rb @@ -96,16 +96,16 @@ describe '#toggle_manager_member_path' do subject { decorator.toggle_manager_member_path } - it { is_expected.to include("members/#{member.user.id}/toggle_manager")} + it { is_expected.to include("members/#{member.id}/toggle_manager")} end describe '#cancel_member_path' do subject { decorator.cancel_member_path } - it { is_expected.to include("members/#{member.user.id}")} + it { is_expected.to include("members/#{member.id}")} end describe '#toggle_active_member_path' do subject { decorator.toggle_active_member_path } - it { is_expected.to include("members/#{member.user.id}/toggle_active")} + it { is_expected.to include("members/#{member.id}/toggle_active")} end end From cd259683aebc35b92aa712c7e652c2d464486fda Mon Sep 17 00:00:00 2001 From: Saverio Trioni Date: Wed, 9 May 2018 03:29:17 +0200 Subject: [PATCH 07/12] Sorting by balance spec wasn't stable, check just th relevant position --- spec/controllers/users_controller_spec.rb | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-) diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 6f87e844c..6fe16f1dc 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -118,16 +118,7 @@ it 'orders the rows by their balance' do get :index, q: { s: "accounts_balance #{direction}" } - expect(assigns(:users).pluck(:id)) - .to eq( - [ - admin_user.id, - user.id, - another_user.id, - wrong_user.id, - empty_email_user.id - ] - ) + expect(assigns(:users).pluck(:id).first).to eq(admin_user.id) end end @@ -137,16 +128,7 @@ it 'orders the rows by their balance' do get :index, q: { s: "accounts_balance #{direction}" } - expect(assigns(:users).pluck(:id)) - .to eq( - [ - user.id, - another_user.id, - wrong_user.id, - empty_email_user.id, - admin_user.id, - ] - ) + expect(assigns(:users).pluck(:id).last).to eq(admin_user.id) end end end From 317db4e7325f62a91bce0c1c303478da83f21c2c Mon Sep 17 00:00:00 2001 From: Saverio Trioni Date: Mon, 14 May 2018 01:49:03 +0200 Subject: [PATCH 08/12] CR suggestions --- Gemfile | 1 - Gemfile.lock | 2 -- app/decorators/member_decorator.rb | 39 +++++++---------------- app/decorators/view_model.rb | 39 +++++++++++++++++++++++ spec/controllers/users_controller_spec.rb | 4 +-- 5 files changed, 53 insertions(+), 32 deletions(-) create mode 100644 app/decorators/view_model.rb diff --git a/Gemfile b/Gemfile index 32c8bda62..88aec66df 100644 --- a/Gemfile +++ b/Gemfile @@ -6,7 +6,6 @@ gem 'rails', '~> 4.2' gem 'rails-i18n' gem "rdiscount" gem 'activeadmin', '~> 1.2.1' -gem 'dry-initializer' gem 'has_scope' gem 'pundit' gem 'pg', '0.17.1' diff --git a/Gemfile.lock b/Gemfile.lock index 66dba6411..fb5b77c75 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -117,7 +117,6 @@ GEM dotenv (1.0.2) dotenv-rails (1.0.2) dotenv (= 1.0.2) - dry-initializer (2.4.0) elasticsearch (1.0.8) elasticsearch-api (= 1.0.7) elasticsearch-transport (= 1.0.7) @@ -382,7 +381,6 @@ DEPENDENCIES database_cleaner (= 1.3.0) devise (~> 4.4.1) dotenv-rails (= 1.0.2) - dry-initializer elasticsearch-model elasticsearch-rails fabrication diff --git a/app/decorators/member_decorator.rb b/app/decorators/member_decorator.rb index 7b5a79ece..ab1b6926a 100644 --- a/app/decorators/member_decorator.rb +++ b/app/decorators/member_decorator.rb @@ -1,18 +1,9 @@ -require 'dry-initializer' - -class MemberDecorator - extend Dry::Initializer - - param :member - param :view_context - - alias_method :v, :view_context - - delegate :user, :member_uid, :active?, to: :member +class MemberDecorator < ViewModel + delegate :user, :member_uid, :active?, to: :object delegate :phone, :alt_phone, :username, to: :user def manager? - !!member.manager + !!object.manager end def row_css_class @@ -20,45 +11,39 @@ def row_css_class end def inactive_icon - v.tag(:span, class: %w[glyphicon glyphicon-time]) unless active? + view.glyph('time') unless active? end def link_to_self - v.link_to(user.username, h.user_path(user)) + view.link_to(user.username, routes.user_path(user)) end def mail_to email = user.unconfirmed_email || user.email - v.mail_to(email) if email && !email.end_with?('example.com') + view.mail_to(email) if email && !email.end_with?('example.com') end def avatar_img - v.image_tag(v.avatar_url(user, 32), width: 32, height: 32) + view.image_tag(view.avatar_url(user, 32), width: 32, height: 32) end def account_balance - v.seconds_to_hm(member.account.try(:balance) || 0) + view.seconds_to_hm(object.account.try(:balance) || 0) end def edit_user_path - h.edit_user_path(user) + routes.edit_user_path(user) end def toggle_manager_member_path - h.toggle_manager_member_path(member) + routes.toggle_manager_member_path(object) end def cancel_member_path - h.member_path(member) + routes.member_path(object) end def toggle_active_member_path - h.toggle_active_member_path(member) - end - - private - - def h - Rails.application.routes.url_helpers + routes.toggle_active_member_path(object) end end diff --git a/app/decorators/view_model.rb b/app/decorators/view_model.rb new file mode 100644 index 000000000..8de925637 --- /dev/null +++ b/app/decorators/view_model.rb @@ -0,0 +1,39 @@ +# View model base class +# --------------------- +# +# Create a subclass to expose some specific methods from a business layer model +# plus view helpers and route helpers. The business object is readable as +# `object`, the view helpers as `view` and the route helpers as `routes`. +# +# Examples +# -------- +# +# class UserDecorator < ViewModel +# def path_to_edit +# routes.edit_user_path(object) +# end +# +# def email_link +# view.mail_to(object.email) +# end +# end +# +# How to use +# ---------- +# +# The first argument to the initializer is an arbitrary object, and the second +# is expected to respond correctly to view helpers like `link_to` and similar. +# +# From controllers, one can pass `self.class.helpers`, and from tests it is +# enough to use ApplicationController.new.view_context. +# +class ViewModel + attr_reader :object, :view, :routes + + def initialize(object, view) + @object = object + @view = view + @routes = Rails.application.routes.url_helpers + end +end + diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 6fe16f1dc..53922082e 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -67,9 +67,9 @@ it 'gets her membership in the current organization' do get :index - assigns(:members).each { |m| expect(m).to be_a(MemberDecorator) } + assigns(:members).each { |m| expect(m).to respond_to(:object) } - expect(assigns(:members).map(&:member)).to eq([ + expect(assigns(:members).map(&:object)).to eq([ member, another_member, member_admin, From edd4797d437f541ffa406ac766bd2f3c86b57a95 Mon Sep 17 00:00:00 2001 From: Saverio Trioni Date: Mon, 14 May 2018 02:37:58 +0200 Subject: [PATCH 09/12] Make the member list view do ransack on members table --- app/controllers/users_controller.rb | 45 ++++++++--------------- app/helpers/application_helper.rb | 5 +++ app/views/users/index.html.erb | 12 +++--- config/environments/test.rb | 2 +- spec/controllers/users_controller_spec.rb | 33 +++++++---------- 5 files changed, 40 insertions(+), 57 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 0f806f399..d5b833364 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -2,36 +2,16 @@ class UsersController < ApplicationController before_filter :authenticate_user! def index - @search = User.ransack(params[:q]) - @search.sorts = 'members_member_uid asc' if @search.sorts.empty? - - @users = @search - .result(distinct: false) - .joins(members: :account) - .eager_load(members: :account) - .where(members: { organization: current_organization.id }) - .page(params[:page]) - .per(25) - - user_ids = @users.pluck(:id) - - @members = current_organization. - members. - where(user_id: user_ids). - includes(:account, :user). - each_with_object({}) { |mem, ob| ob[mem.user_id] = mem }. - values_at(*user_ids). - map { |m| MemberDecorator.new(m, self.class.helpers) } - - # TODO: mutate theparameters so they can be used directly on the - # members table. - # - # @members = current_organization.members. - # joins(:account, :user). - # merge(@search.result(distinct: false)). - # page(params[:page]). - # per(25). - # map { |m| MemberDecorator.new(m, self.class.helpers) } + default_sort + + @search = + current_organization.members.joins(:account, :user).ransack(params[:q]) + + @members = + @search.result.page(params[:page]).per(25) + + @member_view_models = + @members.map { |m| MemberDecorator.new(m, self.class.helpers) } end def show @@ -83,6 +63,11 @@ def update private + def default_sort + params[:q] ||= {} + params[:q][:s] ||= 'member_uid asc' + end + def scoped_users current_organization.users end diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 5c4512e0d..b9b48fedd 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -3,6 +3,11 @@ module ApplicationHelper TEXT_SUCCESS = 'text-success'.freeze TEXT_DANGER = 'text-danger'.freeze + # Ransacks to Members should go to users controller :) + def members_path(*args, &block) + users_path(*args, &block) + end + # from gravatar def avatar_url(user, size = 32) gravatar_id = Digest::MD5::hexdigest(user.email).downcase diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index 6a43807db..bf97a1b1f 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -7,7 +7,7 @@
<%= search_form_for(@search, class: "navbar-form navbar-left") do |f| %> - <%= f.search_field :username_or_email_contains, class: "form-control" %> + <%= f.search_field :user_username_or_user_email_contains, class: "form-control" %> @@ -31,10 +31,10 @@ ID - <%= sort_link @search, :username, User.human_attribute_name(:username) %> + <%= sort_link @search, :user_username, User.human_attribute_name(:username) %> - <%= sort_link @search, :email, User.human_attribute_name(:email) %> + <%= sort_link @search, :user_email, User.human_attribute_name(:email) %> <%= User.human_attribute_name(:phone) %> @@ -43,7 +43,7 @@ <%= User.human_attribute_name(:alt_phone) %> - <%= sort_link @search, 'accounts_balance', Account.human_attribute_name(:balance) %> + <%= sort_link @search, 'account_balance', Account.human_attribute_name(:balance) %> <% if current_user.manages? current_organization %> @@ -54,10 +54,10 @@ - <%= render "user_rows", members: @members %> + <%= render "user_rows", members: @member_view_models %> - <%= paginate @users %> + <%= paginate @members %>
diff --git a/config/environments/test.rb b/config/environments/test.rb index 1037818bc..f76f64fe4 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -43,5 +43,5 @@ # config.action_view.raise_on_missing_translations = true # Avoid seeing all that stuff in tests - config.log_level = :warn + config.log_level = :debug end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 53922082e..27b5b4c54 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -45,7 +45,7 @@ get :index - expect(assigns(:users)).to eq([ + expect(assigns(:members).map(&:user)).to eq([ another_user, admin_user, wrong_user, @@ -67,15 +67,8 @@ it 'gets her membership in the current organization' do get :index - assigns(:members).each { |m| expect(m).to respond_to(:object) } - - expect(assigns(:members).map(&:object)).to eq([ - member, - another_member, - member_admin, - wrong_email_member, - empty_email_member - ]) + expect(assigns(:members)) + .to eq([member, another_member, member_admin, wrong_email_member, empty_email_member]) end it 'shows data for her membership in the current organization' do @@ -89,9 +82,9 @@ login(user) get "index" - expect(assigns(:users)).to eq([user, another_user, - admin_user, wrong_user, - empty_email_user]) + + expect(assigns(:members).map(&:user)) + .to eq([user, another_user, admin_user, wrong_user, empty_email_user]) end end @@ -100,9 +93,9 @@ login(admin_user) get "index" - expect(assigns(:users)).to eq([user, another_user, - admin_user, wrong_user, - empty_email_user]) + + expect(assigns(:members).map(&:user)) + .to eq([user, another_user, admin_user, wrong_user, empty_email_user]) end end @@ -116,9 +109,9 @@ let(:direction) { 'desc' } it 'orders the rows by their balance' do - get :index, q: { s: "accounts_balance #{direction}" } + get :index, q: { s: "account_balance #{direction}" } - expect(assigns(:users).pluck(:id).first).to eq(admin_user.id) + expect(assigns(:members).pluck(:user_id).first).to eq(admin_user.id) end end @@ -126,9 +119,9 @@ let(:direction) { 'asc' } it 'orders the rows by their balance' do - get :index, q: { s: "accounts_balance #{direction}" } + get :index, q: { s: "account_balance #{direction}" } - expect(assigns(:users).pluck(:id).last).to eq(admin_user.id) + expect(assigns(:members).pluck(:user_id).last).to eq(admin_user.id) end end end From 0dabcd1ccecb4c7e9b412d6e2e46e48ebb5726cc Mon Sep 17 00:00:00 2001 From: Saverio Trioni Date: Mon, 14 May 2018 02:54:35 +0200 Subject: [PATCH 10/12] Ransack form can be configured. RTFM. --- app/helpers/application_helper.rb | 5 ----- app/views/users/index.html.erb | 2 +- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index b9b48fedd..5c4512e0d 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -3,11 +3,6 @@ module ApplicationHelper TEXT_SUCCESS = 'text-success'.freeze TEXT_DANGER = 'text-danger'.freeze - # Ransacks to Members should go to users controller :) - def members_path(*args, &block) - users_path(*args, &block) - end - # from gravatar def avatar_url(user, size = 32) gravatar_id = Digest::MD5::hexdigest(user.email).downcase diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index bf97a1b1f..11b4bc2e5 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -6,7 +6,7 @@ organization_path(current_organization) %>
- <%= search_form_for(@search, class: "navbar-form navbar-left") do |f| %> + <%= search_form_for(@search, class: "navbar-form navbar-left", url: users_path) do |f| %> <%= f.search_field :user_username_or_user_email_contains, class: "form-control" %>