From 9a2e03a13b0a6ae3befa7ad10a06ffd51edca533 Mon Sep 17 00:00:00 2001 From: Enrico Stano Date: Thu, 21 Jun 2018 17:04:52 +0200 Subject: [PATCH 01/11] TODO++ --- app/policies/application_policy.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb index 795d32efc..44698d113 100644 --- a/app/policies/application_policy.rb +++ b/app/policies/application_policy.rb @@ -1,6 +1,10 @@ class ApplicationPolicy attr_reader :member, :user, :organization, :record + # TODO: Investigate how to just pass current_user here. + # Probably this will be solved by scoping the resources + # under `/organization`. + # def initialize(member, record) @member = member @user = member.user if member From aa0720663c7d4cf1516b382b21c99eda76843145 Mon Sep 17 00:00:00 2001 From: Enrico Stano Date: Thu, 21 Jun 2018 17:15:33 +0200 Subject: [PATCH 02/11] Rescue ActiveRecord::RecordNotFound and render a 404 --- app/controllers/application_controller.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 5ea40d98e..d9ba1e933 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -17,6 +17,7 @@ class ApplicationController < ActionController::Base end rescue_from Pundit::NotAuthorizedError, with: :user_not_authorized + rescue_from ActiveRecord::RecordNotFound, with: :resource_not_found helper_method :current_organization, :admin?, :superadmin? @@ -118,4 +119,8 @@ def user_not_authorized flash[:error] = "You are not authorized to perform this action." redirect_to(request.referrer || root_path) end + + def resource_not_found + render 'errors/not_found', status: 404 + end end From 7a5fb4cd75d2cb9b0b98a53124f34ca416c81388 Mon Sep 17 00:00:00 2001 From: Enrico Stano Date: Thu, 5 Jul 2018 15:29:16 +0200 Subject: [PATCH 03/11] Doc++ --- app/controllers/posts_controller.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index 220d68715..69f12b357 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -57,6 +57,9 @@ def edit instance_variable_set("@#{resource}", post) end + # GET /offers/:id + # GET /inquiries/:id + # def show scope = if current_user.present? current_organization.posts.active.of_active_members From 4cedf74e0666159e59dbcd1146506adbfa6d8093 Mon Sep 17 00:00:00 2001 From: Enrico Stano Date: Thu, 5 Jul 2018 17:16:39 +0200 Subject: [PATCH 04/11] Load the requested post in any case and specs++ --- app/controllers/posts_controller.rb | 8 +- spec/controllers/offers_controller_spec.rb | 96 +++++++++++++++++----- 2 files changed, 77 insertions(+), 27 deletions(-) diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index 69f12b357..b83033054 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -61,12 +61,8 @@ def edit # GET /inquiries/:id # def show - scope = if current_user.present? - current_organization.posts.active.of_active_members - else - model.all.active.of_active_members - end - post = scope.find params[:id] + post = Post.active.of_active_members.find(params[:id]) + instance_variable_set("@#{resource}", post) end diff --git a/spec/controllers/offers_controller_spec.rb b/spec/controllers/offers_controller_spec.rb index 3c712c0c8..38e86006f 100644 --- a/spec/controllers/offers_controller_spec.rb +++ b/spec/controllers/offers_controller_spec.rb @@ -1,15 +1,15 @@ require "spec_helper" -describe OffersController, type: :controller do - let(:test_organization) { Fabricate(:organization) } - let(:member) { Fabricate(:member, organization: test_organization) } - let(:another_member) { Fabricate(:member, organization: test_organization) } +RSpec.describe OffersController, type: :controller do + let(:organization) { Fabricate(:organization) } + let(:member) { Fabricate(:member, organization: organization) } + let(:another_member) { Fabricate(:member, organization: organization) } let(:yet_another_member) { Fabricate(:member) } let(:test_category) { Fabricate(:category) } let!(:offer) do Fabricate(:offer, user: member.user, - organization: test_organization, + organization: organization, category: test_category) end @@ -59,27 +59,81 @@ end end - describe "GET #show" do - context "with valid params" do - context "with a logged user" do - before { login(another_member.user) } - - it "assigns the requested offer to @offer" do - get :show, id: offer.id - expect(assigns(:offer)).to eq(offer) + describe 'GET #show' do + context 'when the user is logged in' do + before { login(another_member.user) } + + context 'when the requested offer' do + context 'is not active' do + before do + offer.active = false + offer.save! + end + + it 'renders the 404 page' do + get :show, id: offer.id + expect(response.status).to eq(404) + end end - it 'assigns the account destination of the transfer' do - get :show, id: offer.id - expect(assigns(:destination_account)).to eq(member.account) + context 'is active' do + context 'and the user that created the offer is not active anymore' do + before do + member.active = false + member.save! + end + + it 'renders the 404 page' do + get :show, id: offer.id + expect(response.status).to eq(404) + end + end + + context 'and the user that created the offer is active' do + it 'renders a successful response' do + get :show, id: offer.id + expect(response.status).to eq(200) + end + + it 'assigns the requested offer to @offer' do + get :show, id: offer.id + expect(assigns(:offer)).to eq(offer) + end + + it 'assigns the account destination of the transfer' do + get :show, id: offer.id + expect(assigns(:destination_account)).to eq(member.account) + end + + it 'displays the offer\'s user details' do + get :show, id: offer.id + expect(response.body).to include(offer.user.email) + end + end end end + end - context "without a logged in user" do - it "assigns the requested offer to @offer" do - get :show, id: offer.id - expect(assigns(:offer)).to eq(offer) - end + context 'when the user is not a member of the organization where the offer is published' do + let(:another_user) { Fabricate(:user) } + + before { login(another_user) } + + it 'doesn\'t display the offer\'s user details' do + get :show, id: offer.id + expect(response.body).to_not include(offer.user.email) + end + end + + context 'when the user is not logged in' do + it 'assigns the requested offer to @offer' do + get :show, id: offer.id + expect(assigns(:offer)).to eq(offer) + end + + it 'doesn\'t display the offer\'s user details' do + get :show, id: offer.id + expect(response.body).to_not include(offer.user.email) end end end From 8170499e0f186cc0c5d70c17de76aee7f85457b5 Mon Sep 17 00:00:00 2001 From: Enrico Stano Date: Thu, 5 Jul 2018 17:18:30 +0200 Subject: [PATCH 05/11] Switch to post organization if possible --- app/controllers/posts_controller.rb | 17 ++++++++++++ app/views/inquiries/show.html.erb | 19 +++++-------- app/views/offers/show.html.erb | 31 +++++++++------------- app/views/shared/_post.html.erb | 18 +++++++------ app/views/shared/_post_actions.html.erb | 9 +++++++ config/locales/en.yml | 2 +- spec/controllers/offers_controller_spec.rb | 22 +++++++++++++++ 7 files changed, 77 insertions(+), 41 deletions(-) create mode 100644 app/views/shared/_post_actions.html.erb diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index b83033054..71c35f9b1 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -62,6 +62,7 @@ def edit # def show post = Post.active.of_active_members.find(params[:id]) + update_current_organization!(post.organization) instance_variable_set("@#{resource}", post) end @@ -114,4 +115,20 @@ def post_params set_user_id(p) end end + + # TODO: remove this horrible hack ASAP + # + # This hack set the current organization to the post's + # organization, both in session and controller instance variable. + # + # Before changing the current organization it's important to check that + # the current_user is an active member of the organization. + # + # @param organization [Organization] + def update_current_organization!(organization) + return unless current_user && current_user.active?(organization) + + session[:current_organization_id] = organization.id + @current_organization = organization + end end diff --git a/app/views/inquiries/show.html.erb b/app/views/inquiries/show.html.erb index 7c1bb7a0e..388c15283 100644 --- a/app/views/inquiries/show.html.erb +++ b/app/views/inquiries/show.html.erb @@ -1,15 +1,8 @@ -

- <% if admin? || @inquiry.user == current_user %> - <%= link_to edit_inquiry_path(@inquiry), class: "btn btn-warning" do %> - <%= glyph :pencil %> - <%= t "global.edit" %> +<% if @inquiry.organization == current_organization %> +

+ <% if admin? or @inquiry.user == current_user %> + <%= render 'shared/post_actions', post: @inquiry %> <% end %> - <%= link_to @inquiry, - data: { method: :delete, confirm: "sure?" }, - class: "btn btn-danger" do %> - <%= glyph :trash %> - <%= t "global.delete" %> - <% end %> - <% end %> -

+

+<% end %> <%= render "shared/post", post: @inquiry %> diff --git a/app/views/offers/show.html.erb b/app/views/offers/show.html.erb index a6135384a..a3afc8fa3 100644 --- a/app/views/offers/show.html.erb +++ b/app/views/offers/show.html.erb @@ -1,22 +1,15 @@ -

- <% if admin? or @offer.user == current_user %> - <%= link_to edit_offer_path(@offer), class: "btn btn-warning" do %> - <%= glyph :pencil %> - <%= t "global.edit" %> +<% if @offer.organization == current_organization %> +

+ <% if admin? or @offer.user == current_user %> + <%= render 'shared/post_actions', post: @offer %> <% end %> - <%= link_to @offer, - data: { method: :DELETE, confirm: "sure?" }, - class: "btn btn-danger" do %> - <%= glyph :trash %> - <%= t "global.delete" %> + <% if current_user and @offer.user != current_user %> + <%= link_to new_transfer_path(id: @offer.user.id, offer: @offer.id, destination_account_id: @destination_account.id), + class: "btn btn-success" do %> + <%= glyph :time %> + <%= t ".give_time_for" %> + <% end %> <% end %> - <% end %> - <% if current_user and @offer.user != current_user %> - <%= link_to new_transfer_path(id: @offer.user.id, offer: @offer.id, destination_account_id: @destination_account.id), - class: "btn btn-success" do %> - <%= glyph :time %> - <%= t ".give_time_for" %> - <% end %> - <% end %> -

+

+<% end %> <%= render "shared/post", post: @offer %> diff --git a/app/views/shared/_post.html.erb b/app/views/shared/_post.html.erb index d9681bab7..7b755c6d5 100644 --- a/app/views/shared/_post.html.erb +++ b/app/views/shared/_post.html.erb @@ -27,7 +27,7 @@ <% end %> - <% if current_user && current_organization %> + <% if current_user && current_organization == post.organization %>

@@ -60,13 +60,15 @@

-<% unless current_user %> +<% unless post.organization == current_organization %>
- <%= t "posts.show.info", - type: post.class.model_name.human, - organization: post.organization.name %> - <%= link_to t("layouts.application.login"), - new_user_session_path, - class: "btn btn-primary" %> + <%= t 'posts.show.info', + type: post.class.model_name.human, + organization: post.organization.name %> + <% unless current_user %> + <%= link_to t("layouts.application.login"), + new_user_session_path, + class: "btn btn-primary" %> + <% end %>
<% end %> diff --git a/app/views/shared/_post_actions.html.erb b/app/views/shared/_post_actions.html.erb new file mode 100644 index 000000000..eb8a2df0f --- /dev/null +++ b/app/views/shared/_post_actions.html.erb @@ -0,0 +1,9 @@ +<%= link_to post, class: "btn btn-warning" do %> + <%= glyph :pencil %> + <%= t "global.edit" %> +<% end %> + +<%= link_to post, data: { method: :delete, confirm: "sure?" }, class: "btn btn-danger" do %> + <%= glyph :trash %> + <%= t "global.delete" %> +<% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index dfc6deb0e..395f5d472 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -404,7 +404,7 @@ en: title2: Time Banks posts: show: - info: "%{type} of %{organization} to see person's details you have to" + info: "This %{type} belongs to %{organization}." reports: cat_with_users: title: Offered Services diff --git a/spec/controllers/offers_controller_spec.rb b/spec/controllers/offers_controller_spec.rb index 38e86006f..4f1db8ecf 100644 --- a/spec/controllers/offers_controller_spec.rb +++ b/spec/controllers/offers_controller_spec.rb @@ -112,6 +112,28 @@ end end end + + context 'when the user pertains to multiple organizations' do + context 'and user\'s current organization is different than offer\'s organization' do + let(:another_organization) { Fabricate(:organization) } + + before do + Fabricate(:member, user: another_member.user, organization: another_organization) + allow(controller).to receive(:@current_organization).and_return(another_organization) + end + + it 'displays the offer\'s user details' do + get :show, id: offer.id + expect(response.body).to include(offer.user.email) + end + + it 'sets the offer\'s organization as user\'s current organization' do + get :show, id: offer.id + expect(session[:current_organization_id]).to eq(offer.organization_id) + expect(assigns(:current_organization)).to eq(offer.organization) + end + end + end end context 'when the user is not a member of the organization where the offer is published' do From f1f8b80e8855618672c53a99b4440c094e611e2e Mon Sep 17 00:00:00 2001 From: Enrico Stano Date: Thu, 5 Jul 2018 18:37:17 +0200 Subject: [PATCH 06/11] Fix view and specs++ --- app/views/shared/_post.html.erb | 14 +-- spec/views/offers/show.html.erb_spec.rb | 113 ++++++++++++++++++++---- 2 files changed, 105 insertions(+), 22 deletions(-) diff --git a/app/views/shared/_post.html.erb b/app/views/shared/_post.html.erb index 7b755c6d5..370e03362 100644 --- a/app/views/shared/_post.html.erb +++ b/app/views/shared/_post.html.erb @@ -60,15 +60,17 @@ -<% unless post.organization == current_organization %> +<% if !current_user || post.organization != current_organization %>
<%= t 'posts.show.info', type: post.class.model_name.human, organization: post.organization.name %> - <% unless current_user %> - <%= link_to t("layouts.application.login"), - new_user_session_path, - class: "btn btn-primary" %> - <% end %> +
+<% end %> +<% unless current_user %> +
+ <%= link_to t("layouts.application.login"), + new_user_session_path, + class: "btn btn-primary" %>
<% end %> diff --git a/spec/views/offers/show.html.erb_spec.rb b/spec/views/offers/show.html.erb_spec.rb index 65a6e21f3..42b992c78 100644 --- a/spec/views/offers/show.html.erb_spec.rb +++ b/spec/views/offers/show.html.erb_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe 'offers/show' do +RSpec.describe 'offers/show' do let(:organization) { Fabricate(:organization) } let(:member) { Fabricate(:member, organization: organization) } let(:offer) { Fabricate(:offer, user: member.user, organization: organization) } @@ -8,12 +8,10 @@ before do allow(view).to receive(:admin?).and_return(false) - allow(view).to receive(:current_organization) { organization } - allow(offer).to receive(:member).and_return(member) end - context 'when there is logged in' do + context 'when the user is logged in' do let(:logged_user) { Fabricate(:user) } before do @@ -26,25 +24,88 @@ allow(view).to receive(:current_user).and_return(logged_user) end - it 'renders a link to the transfer page' do - assign :offer, offer - assign :destination_account, destination_account - render template: 'offers/show' + context 'when the current organization is the same as offer\'s organization' do + before do + allow(view).to receive(:current_organization) { offer.organization } + end - expect(rendered).to have_link( - t('offers.show.give_time_for'), - href: new_transfer_path( - id: offer.user.id, - offer: offer.id, - destination_account_id: destination_account.id + it 'renders a link to the transfer page' do + assign :offer, offer + assign :destination_account, destination_account + render template: 'offers/show' + + expect(rendered).to have_link( + t('offers.show.give_time_for'), + href: new_transfer_path( + id: offer.user.id, + offer: offer.id, + destination_account_id: destination_account.id + ) ) - ) + end + + it 'displays offer\'s user details' do + assign :offer, offer + assign :destination_account, destination_account + render template: 'offers/show' + + expect(rendered).to include(offer.user.email) + end + end + + context 'when the current organization is not the same as offer\'s organization' do + let(:another_organization) { Fabricate(:organization) } + + before do + Fabricate( + :member, + organization: another_organization, + user: logged_user + ) + + allow(view).to receive(:current_organization) { another_organization } + end + + it 'doesn\'t render a link to the transfer page' do + assign :offer, offer + render template: 'offers/show' + + expect(rendered).to_not have_link( + t('offers.show.give_time_for'), + href: new_transfer_path( + id: offer.user.id, + offer: offer.id, + destination_account_id: destination_account.id + ) + ) + end + + it 'doesn\'t display offer\'s user details' do + assign :offer, offer + assign :destination_account, destination_account + render template: 'offers/show' + + expect(rendered).to_not include(offer.user.email) + end + + it 'displays the offer\'s organization' do + assign :offer, offer + render template: 'offers/show' + + expect(rendered).to include( + t('posts.show.info', + type: offer.class.model_name.human, + organization: offer.organization.name + ) + ) + end end end - context 'where is a guest user' do + context 'when the user is not logged in' do before do allow(view).to receive(:current_user).and_return(nil) + allow(view).to receive(:current_organization).and_return(nil) end it 'does not render a link to the transfer page' do @@ -71,5 +132,25 @@ href: new_user_session_path ) end + + it 'displays the offer\'s organization' do + assign :offer, offer + render template: 'offers/show' + + expect(rendered).to include( + t('posts.show.info', + type: offer.class.model_name.human, + organization: offer.organization.name + ) + ) + end + + it 'doesn\'t display offer\'s user details' do + assign :offer, offer + assign :destination_account, destination_account + render template: 'offers/show' + + expect(rendered).to_not include(offer.user.email) + end end end From fc3fa27c6cbd95ce8654ecc1bd3b696bbd4766e2 Mon Sep 17 00:00:00 2001 From: Marc Anguera Insa Date: Wed, 15 Aug 2018 17:40:07 +0200 Subject: [PATCH 07/11] Re-implement the member_uid searcher by avoiding the column casting thing (gives some problems when sorting the "casted" column as an string). The new strategy: rename the ransacker so the cast only happens when filtering --- app/models/member.rb | 4 ++-- app/views/users/index.html.erb | 2 +- spec/controllers/users_controller_spec.rb | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models/member.rb b/app/models/member.rb index d84d55e5d..ed20524a1 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -1,7 +1,7 @@ class Member < ActiveRecord::Base # Cast the member_uid integer to a string to allow pg ILIKE search (from Ransack *_contains) - ransacker :member_uid do - Arel.sql("to_char(member_uid, '9999999')") + ransacker :member_uid_search do + Arel.sql("member_uid::text") end belongs_to :user diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index 126e72e87..3b4f00312 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -11,7 +11,7 @@
<%= search_form_for(@search, class: "navbar-form navbar-left", url: users_path) do |f| %>
- <%= f.search_field :user_username_or_user_email_or_member_uid_contains, class: "form-control" %> + <%= f.search_field :user_username_or_user_email_or_member_uid_search_contains, class: "form-control" %>