diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index b56fba5c2..75105b40f 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -2,22 +2,13 @@ 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) - - @memberships = current_organization.members. - where(user_id: @users.map(&:id)). - includes(:account).each_with_object({}) do |mem, ob| - ob[mem.user_id] = mem - end + @search = current_organization.members.ransack(search_params) + + @members = + @search.result.eager_load(:account, :user).page(params[:page]).per(25) + + @member_view_models = + @members.map { |m| MemberDecorator.new(m, self.class.helpers) } end def show @@ -69,6 +60,10 @@ def update private + def search_params + {s: 'member_uid asc'}.merge(params.fetch(:q, {})) + end + def scoped_users current_organization.users end diff --git a/app/decorators/member_decorator.rb b/app/decorators/member_decorator.rb new file mode 100644 index 000000000..ab1b6926a --- /dev/null +++ b/app/decorators/member_decorator.rb @@ -0,0 +1,49 @@ +class MemberDecorator < ViewModel + delegate :user, :member_uid, :active?, to: :object + delegate :phone, :alt_phone, :username, to: :user + + def manager? + !!object.manager + end + + def row_css_class + 'bg-danger' unless active? + end + + def inactive_icon + view.glyph('time') unless active? + end + + def link_to_self + view.link_to(user.username, routes.user_path(user)) + end + + def mail_to + email = user.unconfirmed_email || user.email + view.mail_to(email) if email && !email.end_with?('example.com') + end + + def avatar_img + view.image_tag(view.avatar_url(user, 32), width: 32, height: 32) + end + + def account_balance + view.seconds_to_hm(object.account.try(:balance) || 0) + end + + def edit_user_path + routes.edit_user_path(user) + end + + def toggle_manager_member_path + routes.toggle_manager_member_path(object) + end + + def cancel_member_path + routes.member_path(object) + end + + def toggle_active_member_path + 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/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 3bb148ec4..6d53a9f6f 100644 --- a/app/views/users/_user_rows.html.erb +++ b/app/views/users/_user_rows.html.erb @@ -1,62 +1,29 @@ -<% users.each do |user| %> - <% membership = memberships[user.id] %> - - <%= content_tag(:tr, class: membership.active? ? "" : "bg-danger") do %> - - <%= image_tag avatar_url(user, 32), width: 32, height: 32 %> - - <%= membership.member_uid %> +<% members.each do |member| %> + <%= content_tag(:tr, class: member.row_css_class) do %> + <%= member.avatar_img %> + <%= member.member_uid %> - <% if !membership.active? %><% end %> - <%= link_to user.username, user_path(user) %> + <%= member.inactive_icon %> + <%= member.link_to_self %> - - <% if user.unconfirmed_email %> - - <%= user.unconfirmed_email %> - - <% else %> - - <%= user.email %> - - <% end %> - - <%= user.phone %> - <%= user.alt_phone %> - <%= seconds_to_hm(membership.account.try(:balance) || mdash) %> + <%= member.mail_to %> + <%= member.phone %> + <%= member.alt_phone %> + <%= member.account_balance %> <% if current_user.manages?(current_organization) %> - <%= link_to edit_user_path(user), class: "action" do %> + <%= link_to member.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 %> - <% if !!membership.manager %> - <%= glyph :arrow_down %> - <%= t "global.demote" %> - <% else %> - <%= glyph :arrow_up %> - <%= t "global.promote" %> - <% end %> - <% end %> + <% if member.active? %> + <%= render 'toggle_manager_link', member: member if can_toggle_manager?(member) %> <% else %> - <%= link_to cancel_member_path(membership), class: "action", data: { confirm: t('.cancel_warning', user: user.username) }, method: :delete do %> - <%= glyph :ban_circle %> - <%= t("global.cancel_membership") %> - <% end %> + <%= render 'cancel_membership_link', member: member if can_cancel_member?(member) %> <% end %> - <%= link_to toggle_active_member_path(membership), class: "action", method: :put, data: { confirm: t('users.index.active_warning', username: user.username) } do %> - <% if membership.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/app/views/users/index.html.erb b/app/views/users/index.html.erb index fea03f0d9..11b4bc2e5 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -6,8 +6,8 @@ organization_path(current_organization) %> diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 9f5df70e2..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,13 +67,8 @@ 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 @@ -87,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 @@ -98,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 @@ -114,18 +109,9 @@ let(:direction) { 'desc' } 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 - ] - ) + get :index, q: { s: "account_balance #{direction}" } + + expect(assigns(:members).pluck(:user_id).first).to eq(admin_user.id) end end @@ -133,18 +119,9 @@ let(:direction) { 'asc' } 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, - ] - ) + get :index, q: { s: "account_balance #{direction}" } + + expect(assigns(:members).pluck(:user_id).last).to eq(admin_user.id) end end end diff --git a/spec/decorators/member_decorator_spec.rb b/spec/decorators/member_decorator_spec.rb new file mode 100644 index 000000000..006596390 --- /dev/null +++ b/spec/decorators/member_decorator_spec.rb @@ -0,0 +1,111 @@ +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 } + + 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 + subject { decorator.link_to_self } + it { is_expected.to match("members/#{member.user.id}")} + 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 } + 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.id}/toggle_manager")} + end + + describe '#cancel_member_path' do + subject { decorator.cancel_member_path } + 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.id}/toggle_active")} + end +end