From 5a3ec59721804ed8bf1883999c2e55bb51d444a1 Mon Sep 17 00:00:00 2001 From: Piotr Janik Date: Mon, 25 Mar 2024 17:27:01 +0000 Subject: [PATCH] chore: refactor policies, add specs [PT-187185134] --- rails/app/policies/portal/clazz_policy.rb | 24 +++--- rails/app/policies/portal/offering_policy.rb | 25 +++--- .../spec/policies/portal/clazz_policy_spec.rb | 85 +++++++++++++++++++ 3 files changed, 106 insertions(+), 28 deletions(-) diff --git a/rails/app/policies/portal/clazz_policy.rb b/rails/app/policies/portal/clazz_policy.rb index 7688405e88..fa4c0cb26e 100644 --- a/rails/app/policies/portal/clazz_policy.rb +++ b/rails/app/policies/portal/clazz_policy.rb @@ -1,23 +1,19 @@ class Portal::ClazzPolicy < ApplicationPolicy class Scope < Scope def resolve - if user && user.has_role?('admin') + return none unless user + + if user.has_role?('admin') all - elsif user && (user.is_project_admin? || user.is_project_researcher?) + elsif user.is_project_admin? || user.is_project_researcher? # prevents a bunch of unnecessary model loads by not using the user#admin_for_project_teachers and user#researcher_for_project_teachers methods - teacher_scope = Pundit.policy_scope(user, Portal::Teacher) - teacher_clazz_ids = teacher_scope - .joins("INNER JOIN portal_teacher_clazzes __ptc_scope ON __ptc_scope.teacher_id = portal_teachers.id") + teacher_ids_subquery = Pundit.policy_scope(user, Portal::Teacher).select(:id) + scope + .joins("INNER JOIN portal_teacher_clazzes ON portal_teacher_clazzes.clazz_id = portal_clazzes.id") + .where(portal_teacher_clazzes: { teacher_id: teacher_ids_subquery }) .distinct - .pluck("__ptc_scope.clazz_id") - if teacher_clazz_ids.length > 0 - scope.where(id: teacher_clazz_ids) - else - none - end - elsif user && user.portal_teacher - clazz_ids = user.portal_teacher.clazz_ids - scope.where(id: clazz_ids) + elsif user.portal_teacher + scope.where(id: user.portal_teacher.clazz_ids) else none end diff --git a/rails/app/policies/portal/offering_policy.rb b/rails/app/policies/portal/offering_policy.rb index 8efb66ac7b..e3f67346c7 100644 --- a/rails/app/policies/portal/offering_policy.rb +++ b/rails/app/policies/portal/offering_policy.rb @@ -10,23 +10,20 @@ def api_index? class Scope < Scope def resolve - if user && user.has_role?('admin') + return none unless user + + if user.has_role?('admin') all - elsif user && (user.is_project_admin? || user.is_project_researcher?) + elsif user.is_project_admin? || user.is_project_researcher? # prevents a bunch of unnecessary model loads by not using the user#admin_for_project_teachers and user#researcher_for_project_teachers methods - teacher_scope = Pundit.policy_scope(user, Portal::Teacher) - teacher_clazz_ids = teacher_scope - .joins("INNER JOIN portal_teacher_clazzes __ptc_scope ON __ptc_scope.teacher_id = portal_teachers.id") + teacher_ids_subquery = Pundit.policy_scope(user, Portal::Teacher).select(:id) + scope + .joins("INNER JOIN portal_teacher_clazzes ON portal_teacher_clazzes.clazz_id = portal_offerings.clazz_id") + .where(portal_teacher_clazzes: { teacher_id: teacher_ids_subquery }) .distinct - .pluck("__ptc_scope.clazz_id") - if teacher_clazz_ids.length > 0 - scope.where(clazz_id: teacher_clazz_ids) - else - none - end - elsif user && user.portal_teacher - clazz_ids = user.portal_teacher.clazz_ids - scope.where(clazz_id: clazz_ids) + + elsif user.portal_teacher + scope.where(clazz_id: user.portal_teacher.clazz_ids) else none end diff --git a/rails/spec/policies/portal/clazz_policy_spec.rb b/rails/spec/policies/portal/clazz_policy_spec.rb index 17215c1190..b47848816a 100644 --- a/rails/spec/policies/portal/clazz_policy_spec.rb +++ b/rails/spec/policies/portal/clazz_policy_spec.rb @@ -3,5 +3,90 @@ require 'spec_helper' RSpec.describe Portal::ClazzPolicy do + let(:user) { FactoryBot.create(:user) } + let(:scope) { Pundit.policy_scope!(user, Portal::Clazz) } + describe "Scope" do + before(:each) do + @project1 = FactoryBot.create(:project) + @project2 = FactoryBot.create(:project) + @project3 = FactoryBot.create(:project) + + @cohort1 = FactoryBot.create(:admin_cohort) + @cohort2 = FactoryBot.create(:admin_cohort) + @cohort3 = FactoryBot.create(:admin_cohort) + + @project1.cohorts << @cohort1 + @project2.cohorts << @cohort2 + @project3.cohorts << @cohort3 + + @teacher1 = FactoryBot.create(:portal_teacher) + @teacher2 = FactoryBot.create(:portal_teacher) + @teacher3 = FactoryBot.create(:portal_teacher) + + @runnable1 = FactoryBot.create(:external_activity) + @runnable2 = FactoryBot.create(:external_activity) + @runnable3 = FactoryBot.create(:external_activity) + + @teacher1.cohorts << @cohort1 + @teacher2.cohorts << @cohort1 + @teacher3.cohorts << @cohort2 + + @clazz1 = @teacher1.clazzes[0] + @clazz2 = @teacher2.clazzes[0] + @clazz3 = @teacher3.clazzes[0] + end + + context 'normal user' do + it 'does not allow access to any classes' do + expect(scope.to_a.length).to eq 0 + end + end + + context 'project researcher' do + before(:each) do + user.add_role_for_project('researcher', @project1) + end + + it 'allows access to project classes' do + expect(scope.to_a).to match_array([@clazz1, @clazz2]) + end + end + + context 'project admin' do + before(:each) do + user.add_role_for_project('admin', @project2) + end + + it 'allows access to project classes' do + expect(scope.to_a).to match_array([@clazz3]) + end + + end + + context 'teacher' do + let(:user) { @teacher1.user } + it 'allows access to teacher classes' do + expect(scope.to_a).to match_array([@clazz1]) + end + context 'who is also a project admin' do + before(:each) do + # project3 is for @cohort3 and has no teachers in it. + user.add_role_for_project('admin', @project3) + end + it 'allows access to teacher classes' do + # We still expect to see the teachers own classes here + # Even though they are not an admin for @project1 + expect(scope.to_a).to match_array([@clazz1]) + end + end + end + + context 'admin user' do + let(:user) { FactoryBot.generate(:admin_user) } + it 'allows access to all classes' do + expect(scope.to_a).to match_array([@clazz1, @clazz2, @clazz3]) + end + end + end end