diff --git a/app/controllers/admin/base_controller.rb b/app/controllers/admin/base_controller.rb index ab9719bd..8c2db1f4 100644 --- a/app/controllers/admin/base_controller.rb +++ b/app/controllers/admin/base_controller.rb @@ -13,10 +13,10 @@ module Admin redirect_to sign_in_path return false end - unless (current_user.has_role? :organizer, :any) || (current_user.has_role? :cfp, :any) || - (current_user.has_role? :info_desk, :any) || (current_user.has_role? :organization_admin, :any) || - (current_user.has_role? :volunteers_coordinator, :any) || - (current_user.has_role? :track_organizer, :any) || current_user.is_admin + unless (current_user.has_cached_role? :organizer, :any) || (current_user.has_cached_role? :cfp, :any) || + (current_user.has_cached_role? :info_desk, :any) || (current_user.has_cached_role? :organization_admin, :any) || + (current_user.has_cached_role? :volunteers_coordinator, :any) || + (current_user.has_cached_role? :track_organizer, :any) || current_user.is_admin raise CanCan::AccessDenied.new('You are not authorized to access this page.') end end diff --git a/app/controllers/admin/organizations_controller.rb b/app/controllers/admin/organizations_controller.rb index d125c5cf..8562d61a 100644 --- a/app/controllers/admin/organizations_controller.rb +++ b/app/controllers/admin/organizations_controller.rb @@ -45,7 +45,7 @@ module Admin end def assign_org_admins - if @user.has_role? 'organization_admin', @organization + if @user.has_cached_role? 'organization_admin', @organization flash[:error] = "User #{@user.email} already has the role organization admin" elsif @user.add_role 'organization_admin', @organization flash[:notice] = "Successfully added role organization admin to user #{@user.email}" diff --git a/app/controllers/admin/roles_controller.rb b/app/controllers/admin/roles_controller.rb index 7ddb7953..ab7be841 100644 --- a/app/controllers/admin/roles_controller.rb +++ b/app/controllers/admin/roles_controller.rb @@ -86,7 +86,7 @@ module Admin else flash[:error] = "Could not remove role #{@role.name} from user #{user.email}" end - elsif user.has_role? @role.name, role_resource + elsif user.has_cached_role? @role.name, role_resource flash[:error] = "User #{user.email} already has the role #{@role.name}" # Add user elsif user.add_role @role.name, role_resource diff --git a/app/controllers/admin/versions_controller.rb b/app/controllers/admin/versions_controller.rb index 5056f3ff..5f18da1c 100644 --- a/app/controllers/admin/versions_controller.rb +++ b/app/controllers/admin/versions_controller.rb @@ -6,7 +6,7 @@ module Admin def index @conferences_with_role = current_user.is_admin? ? Conference.pluck(:short_title) : Conference.with_role([:organizer, :cfp, :info_desk], current_user).pluck(:short_title) - if current_user.has_role? :organization_admin, :any + if current_user.has_cached_role? :organization_admin, :any @conferences_with_role = Organization.with_role('organization_admin', current_user).map { |org| org.conferences.pluck :short_title }.flatten end @conferences_with_role.uniq! diff --git a/app/helpers/admin/volunteers_helper.rb b/app/helpers/admin/volunteers_helper.rb index 0f6fa305..6115f15b 100644 --- a/app/helpers/admin/volunteers_helper.rb +++ b/app/helpers/admin/volunteers_helper.rb @@ -1,7 +1,7 @@ module Admin module VolunteersHelper def can_manage_volunteers?(conference) - current_user.has_role?(:organizer, conference) || current_user.has_role?(:volunteers_coordinator, conference) + current_user.has_cached_role?(:organizer, conference) || current_user.has_cached_role?(:volunteers_coordinator, conference) end end end diff --git a/app/models/admin_ability.rb b/app/models/admin_ability.rb index cc318da2..603f499e 100644 --- a/app/models/admin_ability.rb +++ b/app/models/admin_ability.rb @@ -5,8 +5,8 @@ class AdminAbility # Order Abilities # (Check https://github.com/CanCanCommunity/cancancan/wiki/Ability-Precedence) # Check roles of user, using rolify. Role name is *case sensitive* - # user.is_organizer? or user.has_role? :organizer - # user.is_cfp_of? Conference or user.has_role? :cfp, Conference + # user.is_organizer? or user.has_cached_role? :organizer + # user.is_cfp_of? Conference or user.has_cached_role? :cfp, Conference # user.is_info_desk_of? Conference # user.is_volunteers_coordinator_of? Conference # user.is_attendee_of? Conference @@ -79,12 +79,12 @@ class AdminAbility # Abilities for signed in users with roles def signed_in_with_roles(user) - signed_in_with_organization_admin_role(user) if user.has_role? :organization_admin, :any - signed_in_with_organizer_role(user) if user.has_role? :organizer, :any - signed_in_with_cfp_role(user) if user.has_role? :cfp, :any - signed_in_with_info_desk_role(user) if user.has_role? :info_desk, :any - signed_in_with_volunteers_coordinator_role(user) if user.has_role? :volunteers_coordinator, :any - signed_in_with_track_organizer_role(user) if user.has_role? :track_organizer, :any + signed_in_with_organization_admin_role(user) if user.has_cached_role? :organization_admin, :any + signed_in_with_organizer_role(user) if user.has_cached_role? :organizer, :any + signed_in_with_cfp_role(user) if user.has_cached_role? :cfp, :any + signed_in_with_info_desk_role(user) if user.has_cached_role? :info_desk, :any + signed_in_with_volunteers_coordinator_role(user) if user.has_cached_role? :volunteers_coordinator, :any + signed_in_with_track_organizer_role(user) if user.has_cached_role? :track_organizer, :any common_abilities_for_roles(user) end diff --git a/app/models/user.rb b/app/models/user.rb index 05c73d19..cb09e78c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -6,6 +6,9 @@ end class User < ApplicationRecord rolify + # prevent N+1 queries with has_cached_role? by preloading roles *always* + default_scope { preload(:roles) } + has_many :physical_tickets, through: :ticket_purchases do def by_conference(conference) where('ticket_purchases.conference_id = ?', conference) diff --git a/app/views/tracks/index.html.haml b/app/views/tracks/index.html.haml index b20a3ca9..23310dc3 100644 --- a/app/views/tracks/index.html.haml +++ b/app/views/tracks/index.html.haml @@ -73,7 +73,7 @@ method: :patch, class: 'btn btn-mini btn-success', id: "resubmit_track_request_#{track.id}" - if can? :edit, track = link_to 'Edit', edit_conference_program_track_path(@conference.short_title, track), class: 'btn btn-default' - - if current_user.has_role? :track_organizer, track + - if current_user.has_cached_role? :track_organizer, track = link_to 'Manage', admin_conference_program_track_path(@conference.short_title, track), class: 'btn btn-default' .row diff --git a/app/views/tracks/show.html.haml b/app/views/tracks/show.html.haml index c81d60ec..52448b98 100644 --- a/app/views/tracks/show.html.haml +++ b/app/views/tracks/show.html.haml @@ -8,7 +8,7 @@ .btn-group.pull-right - if can? :edit, @track = link_to 'Edit Track request', edit_conference_program_track_path(@conference.short_title, @track), class: 'btn btn-primary' - - if current_user.has_role? :track_organizer, @track + - if current_user.has_cached_role? :track_organizer, @track = link_to 'Manage', admin_conference_program_track_path(@conference.short_title, @track), class: 'btn btn-default' .row .col-md-8 diff --git a/spec/features/conference_spec.rb b/spec/features/conference_spec.rb index a7a44c3e..bfd0be39 100644 --- a/spec/features/conference_spec.rb +++ b/spec/features/conference_spec.rb @@ -30,7 +30,8 @@ feature Conference do .to eq('Conference was successfully created.') expect(Conference.count).to eq(expected_count) expect(Conference.last.organization).to eq(organization) - expect(user.has_role? :organizer, Conference.last).to eq(true) + user.reload + expect(user.has_cached_role? :organizer, Conference.last).to eq(true) end scenario 'update conference', feature: true, js: true do diff --git a/spec/features/roles_spec.rb b/spec/features/roles_spec.rb index ba9e5d4c..00d15006 100644 --- a/spec/features/roles_spec.rb +++ b/spec/features/roles_spec.rb @@ -58,7 +58,7 @@ feature Role do click_button 'Add' user_with_no_role.reload - expect(user_with_no_role.has_role?(role.name, conference)).to eq true + expect(user_with_no_role.has_cached_role?(role.name, conference)).to eq true end scenario "removes role #{role_name}", feature: true, js: true do @@ -69,7 +69,8 @@ feature Role do expect(find('.alert').text).to eq "×Successfully removed role #{role_name} from user #{user_with_role.email}" expect(by_role_name).to eq(role_name) | eq('organizer') - expect(user_with_role.has_role?(role_name, conference)).to eq false + user_with_role.reload + expect(user_with_role.has_cached_role?(role_name, conference)).to eq false end end @@ -118,14 +119,15 @@ feature Role do click_button 'Add' user_with_no_role.reload - expect(user_with_no_role.has_role?('organization_admin', organization)).to eq true + expect(user_with_no_role.has_cached_role?('organization_admin', organization)).to eq true end scenario 'successfully removes role organization_admin' do click_link('Admins', href: admins_admin_organization_path(organization.id)) first('tr').find('.btn-danger').click - expect(organization_admin.has_role?('organization_admin', organization)).to eq false + organization_admin.reload + expect(organization_admin.has_cached_role?('organization_admin', organization)).to eq false end end diff --git a/spec/models/track_spec.rb b/spec/models/track_spec.rb index 8f53c732..10b92462 100644 --- a/spec/models/track_spec.rb +++ b/spec/models/track_spec.rb @@ -347,17 +347,17 @@ describe Track do end it 'gives the role of the track organizer to the submitter of the track' do - expect(@submitter.has_role?(:track_organizer, self_organized_track)).to eq false + expect(@submitter.has_cached_role?(:track_organizer, self_organized_track)).to eq false self_organized_track.assign_role_to_submitter - expect(@submitter.has_role?(:track_organizer, self_organized_track)).to eq true + expect(@submitter.has_cached_role?(:track_organizer, self_organized_track)).to eq true end it 'is executed when the track is confirmed' do self_organized_track.state = 'accepted' self_organized_track.save! - expect(@submitter.has_role?(:track_organizer, self_organized_track)).to eq false + expect(@submitter.has_cached_role?(:track_organizer, self_organized_track)).to eq false self_organized_track.confirm - expect(@submitter.has_role?(:track_organizer, self_organized_track)).to eq true + expect(@submitter.has_cached_role?(:track_organizer, self_organized_track)).to eq true end end @@ -374,9 +374,10 @@ describe Track do end it 'revokes the role of the track organizer' do - expect(@a_track_organizer.has_role?(:track_organizer, self_organized_track)).to eq true + expect(@a_track_organizer.has_cached_role?(:track_organizer, self_organized_track)).to eq true self_organized_track.revoke_role_and_cleanup - expect(@a_track_organizer.has_role?(:track_organizer, self_organized_track)).to eq false + @a_track_organizer.reload + expect(@a_track_organizer.has_cached_role?(:track_organizer, self_organized_track)).to eq false end it 'destroys the track\'s schedules' do @@ -403,14 +404,16 @@ describe Track do self_organized_track.state = 'confirmed' self_organized_track.save! self_organized_track.cancel - expect(@a_track_organizer.has_role?(:track_organizer, self_organized_track)).to eq false + @a_track_organizer.reload + expect(@a_track_organizer.has_cached_role?(:track_organizer, self_organized_track)).to eq false @event_of_self_organized_track.reload expect(@event_of_self_organized_track.track).to eq nil end it 'is executed when the track is withdrawn' do self_organized_track.withdraw - expect(@a_track_organizer.has_role?(:track_organizer, self_organized_track)).to eq false + @a_track_organizer.reload + expect(@a_track_organizer.has_cached_role?(:track_organizer, self_organized_track)).to eq false @event_of_self_organized_track.reload expect(@event_of_self_organized_track.track).to eq nil end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index b5b63089..019263ac 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -395,22 +395,22 @@ describe User do expect(another_user.roles[1]).to eq(cfp_role) end - describe '#has_role?' do + describe '#has_cached_role?' do describe 'when user has a role' do it 'returns true when the user has the role' do user = create(:user, role_ids: organizer_role.id) - expect(user.has_role?('organizer', conference)).to be true + expect(user.has_cached_role?('organizer', conference)).to be true end it 'returns false when the user does not have the role' do user = create(:user, role_ids: cfp_role.id) - expect(user.has_role?('organizer', conference)).to be false + expect(user.has_cached_role?('organizer', conference)).to be false end end it 'returns false when the user does not have a role' do user = create(:user, role_ids: []) - expect(user.has_role?('organizer', conference)).to be false + expect(user.has_cached_role?('organizer', conference)).to be false end end end