Merge pull request #1253 from concord-consortium/187185134-refactor-p…

…olicies

Refactor Clazz and Offering policies, add specs

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

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

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