From 5414204d0b99e9d187dec596cabd765425f253d6 Mon Sep 17 00:00:00 2001 From: Piotr Janik Date: Thu, 4 Jul 2024 21:19:39 +0000 Subject: [PATCH] fix(PermissionForms): fix class_permission_forms? policy, add lots of API specs [PT-187613639] --- .../api/v1/permission_forms_controller.rb | 4 - rails/app/models/user.rb | 6 +- rails/app/policies/portal/clazz_policy.rb | 2 +- .../v1/permission_forms_controller_spec.rb | 296 +++++++++++++----- rails/spec/models/user_spec.rb | 28 ++ 5 files changed, 254 insertions(+), 82 deletions(-) diff --git a/rails/app/controllers/api/v1/permission_forms_controller.rb b/rails/app/controllers/api/v1/permission_forms_controller.rb index a7393620c..8e968ac5e 100644 --- a/rails/app/controllers/api/v1/permission_forms_controller.rb +++ b/rails/app/controllers/api/v1/permission_forms_controller.rb @@ -164,10 +164,6 @@ def bulk_update end render json: { message: "Bulk update successful" } - rescue ActiveRecord::RecordNotFound => e - render json: { error: e.message }, status: :not_found - rescue => e - render json: { error: e.message }, status: :unprocessable_entity end private diff --git a/rails/app/models/user.rb b/rails/app/models/user.rb index 3a618a49b..67ad9bd27 100644 --- a/rails/app/models/user.rb +++ b/rails/app/models/user.rb @@ -454,10 +454,12 @@ def is_project_member?(project=nil) is_project_admin?(project) || is_project_researcher?(project) || is_project_cohort_member?(project) end - def is_researcher_for_clazz?(clazz) + def is_researcher_for_clazz?(clazz, check_can_manage_permission_forms: false) # check if class has teacher in a cohort of a project the user is a researcher of using a explicit join to avoid a # bunch of unneeded object instantiation - researcher_for_projects + projects_scope = check_can_manage_permission_forms ? researcher_for_projects.where("can_manage_permission_forms = ?", true) : researcher_for_projects + + projects_scope .joins("INNER JOIN admin_cohorts __ac ON __ac.project_id = admin_projects.id") .joins("INNER JOIN admin_cohort_items __aci ON __aci.admin_cohort_id = __ac.id AND __aci.item_type = 'Portal::Teacher'") .joins("INNER JOIN portal_teachers __pt ON __pt.id = __aci.item_id") diff --git a/rails/app/policies/portal/clazz_policy.rb b/rails/app/policies/portal/clazz_policy.rb index 0a2af51d7..5f688c9a2 100644 --- a/rails/app/policies/portal/clazz_policy.rb +++ b/rails/app/policies/portal/clazz_policy.rb @@ -53,7 +53,7 @@ def external_report? # Used by Portal::API::V1::PermissionFormsController: def class_permission_forms? - admin? || class_teacher? || class_project_admin? || class_researcher? + admin? || class_teacher? || class_project_admin? || (user && record && user.is_researcher_for_clazz?(record, check_can_manage_permission_forms: true)) end private diff --git a/rails/spec/controllers/api/v1/permission_forms_controller_spec.rb b/rails/spec/controllers/api/v1/permission_forms_controller_spec.rb index 53eb03ce8..d7884fc74 100644 --- a/rails/spec/controllers/api/v1/permission_forms_controller_spec.rb +++ b/rails/spec/controllers/api/v1/permission_forms_controller_spec.rb @@ -1,48 +1,73 @@ require 'spec_helper' RSpec.describe API::V1::PermissionFormsController, type: :controller do + let (:project) { FactoryBot.create(:project) } + let (:another_project) { FactoryBot.create(:project) } let(:admin) { FactoryBot.generate(:admin_user) } + let (:project_admin) { FactoryBot.generate(:author_user) } + let (:project_researcher) { FactoryBot.generate(:author_user) } + + let (:clazz) { FactoryBot.create(:portal_clazz) } + let (:teacher) { FactoryBot.create(:teacher, user: FactoryBot.create(:user, first_name: 'John', last_name: 'Doe', login: 'test_teacher', email: 'test_teacher_email@mail.com')) } + let (:cohort) { FactoryBot.create(:admin_cohort, project: project) } + let (:student) { FactoryBot.create(:full_portal_student) } + let (:permission_form) { FactoryBot.create(:permission_form, project: project, name: 'Test Form', url: 'http://example.com') } + let (:another_permission_form) { FactoryBot.create(:permission_form, project: another_project, name: 'Another Form', url: 'http://another.com') } before do sign_in admin + + teacher.cohorts << cohort + clazz.teachers << teacher + clazz.students << student + student.permission_forms << permission_form + student.permission_forms << another_permission_form end - describe 'get an ok response from the index endpoint' do - it 'GET index' do + describe 'GET index' do + it 'returns an empty array if there are no permission forms' do + permission_form.destroy + another_permission_form.destroy get :index expect(response).to have_http_status(:ok) + expect(JSON.parse(response.body)).to eq([]) end it 'returns a list of permission forms including permissions' do - Portal::PermissionForm.create!(name: 'Test Form', url: 'http://example.com', project_id: 1) get :index expect(response).to have_http_status(:ok) expect(JSON.parse(response.body)).to match([ - hash_including('name' => 'Test Form', 'url' => 'http://example.com', 'project_id' => 1, 'is_archived' => false, 'can_delete' => true) + hash_including({ 'name' => 'Test Form', 'url' => 'http://example.com', 'project_id' => project.id, 'is_archived' => false, 'can_delete' => true }), + hash_including({ 'name' => 'Another Form', 'url' => 'http://another.com', 'project_id' => another_project.id, 'is_archived' => false, 'can_delete' => true }) ]) end end - it 'POST create creates a new permission form' do - post :create, params: { permission_form: { name: 'Test Form', url: 'http://example.com', project_id: 1 } } - expect(response).to have_http_status(:ok) - expect(response.body).to include('Test Form') - expect(Portal::PermissionForm.first.name).to eq('Test Form') + describe 'POST create' do + it 'creates a new permission form' do + post :create, params: { permission_form: { name: 'Test Form 3', url: 'http://example.com', project_id: project.id } } + expect(response).to have_http_status(:ok) + expect(response.body).to include('Test Form 3') + expect(Portal::PermissionForm.last.name).to eq('Test Form 3') + end end - it 'PUT update updates the permission form' do - permission_form = Portal::PermissionForm.create!(name: 'Test Form', url: 'http://example.com', project_id: 1) - put :update, params: { id: permission_form.id, permission_form: { name: 'New Name' } } - expect(response).to have_http_status(:ok) - expect(Portal::PermissionForm.find(permission_form.id).name).to eq('New Name') + + describe 'PUT update' do + it 'updates the permission form' do + put :update, params: { id: permission_form.id, permission_form: { name: 'New Name' } } + expect(response).to have_http_status(:ok) + expect(Portal::PermissionForm.find(permission_form.id).name).to eq('New Name') + end end - it 'DELETE destroy deletes the permission form' do - permission_form = Portal::PermissionForm.create!(name: 'Test Form', url: 'http://example.com', project_id: 1) - expect(Portal::PermissionForm.count).to eq(1) - delete :destroy, params: { id: permission_form.id } - expect(response).to have_http_status(:ok) - expect(Portal::PermissionForm.count).to eq(0) + describe 'DELETE destroy' do + it 'deletes the permission form' do + expect(Portal::PermissionForm.count).to eq(2) + delete :destroy, params: { id: permission_form.id } + expect(response).to have_http_status(:ok) + expect(Portal::PermissionForm.count).to eq(1) + end end describe 'GET search_teachers' do @@ -53,7 +78,6 @@ end it 'returns a list of teachers with login matching the name' do - teacher = FactoryBot.create(:teacher, user: FactoryBot.create(:user, login: 'test_teacher')) get :search_teachers, params: { name: 'test_teacher' } expect(response).to have_http_status(:ok) expect(JSON.parse(response.body)["teachers"]).to match([ @@ -62,8 +86,7 @@ end it 'returns a list of teachers with email matching the name' do - teacher = FactoryBot.create(:teacher, user: FactoryBot.create(:user, email: 'test_teacher@mail.com')) - get :search_teachers, params: { name: 'test_teacher' } + get :search_teachers, params: { name: 'test_teacher_email' } expect(response).to have_http_status(:ok) expect(JSON.parse(response.body)["teachers"]).to match([ hash_including('id' => teacher.id, 'name' => teacher.user.name, 'email' => teacher.user.email, 'login' => teacher.user.login) @@ -71,7 +94,6 @@ end it 'returns a list of teachers with first or last name matching the name' do - teacher = FactoryBot.create(:teacher, user: FactoryBot.create(:user, first_name: 'John', last_name: 'Doe')) get :search_teachers, params: { name: 'John' } expect(response).to have_http_status(:ok) expect(JSON.parse(response.body)["teachers"]).to match([ @@ -86,7 +108,6 @@ end it 'escapes provided name parameter' do - teacher = FactoryBot.create(:teacher, user: FactoryBot.create(:user, login: 'test_teacher')) get :search_teachers, params: { name: 't__t_teacher' } expect(response).to have_http_status(:ok) expect(JSON.parse(response.body)["teachers"]).to eq([]) @@ -97,8 +118,8 @@ end it 'returns a list of teachers with provided limit' do - FactoryBot.create_list(:teacher, 5, user: FactoryBot.create(:user, login: 'test_teacher')) - get :search_teachers, params: { name: 'test_teacher', limit: 3 } + FactoryBot.create_list(:teacher, 5, user: FactoryBot.create(:user, first_name: 'Mark')) + get :search_teachers, params: { name: 'Mark', limit: 3 } expect(response).to have_http_status(:ok) expect(JSON.parse(response.body)["teachers"].size).to eq(3) expect(JSON.parse(response.body)["limit_applied"]).to eq(true) @@ -106,48 +127,97 @@ end end - describe 'when user is a project admin' do - let (:project) { FactoryBot.create(:project) } - let (:another_project) { FactoryBot.create(:project) } - let (:project_admin) { FactoryBot.generate(:author_user) } + describe 'GET projects' do + it 'returns a list of projects the user can manage permission forms for' do + get :projects + expect(response).to have_http_status(:ok) + expect(JSON.parse(response.body).size).to eq(2) + end + end + + describe 'GET class_permission_forms' do + it 'returns a list of students with their permission forms' do + get :class_permission_forms, params: { class_id: clazz.id } + expect(response).to have_http_status(:ok) + expect(JSON.parse(response.body)).to match([ + hash_including('id' => student.id, 'name' => student.user.name, 'login' => student.user.login, 'permission_forms' => [ + hash_including('id' => permission_form.id, 'name' => permission_form.name, 'url' => permission_form.url, 'project_id' => permission_form.project_id, 'is_archived' => false), + hash_including('id' => another_permission_form.id, 'name' => another_permission_form.name, 'url' => another_permission_form.url, 'project_id' => another_permission_form.project_id, 'is_archived' => false) + ]) + ]) + end + end + + describe 'POST bulk_update' do + it 'adds and removes permission forms for a list of students' do + permission_form_to_add = FactoryBot.create(:permission_form) + post :bulk_update, params: { + class_id: clazz.id, + student_ids: [student.id], + add_permission_form_ids: [permission_form_to_add.id], + remove_permission_form_ids: [permission_form.id] + } + expect(response).to have_http_status(:ok) + expect(student.reload.permission_forms).to include(permission_form_to_add) + expect(student.reload.permission_forms).not_to include(permission_form) + end + + it 'returns an error if any student does not belong to the specified class' do + student.clazzes = [] + student.save + + post :bulk_update, params: { + class_id: clazz.id, + student_ids: [student.id], + add_permission_form_ids: [permission_form.id] + } + + expect(response).to have_http_status(:unprocessable_entity) + expect(JSON.parse(response.body)['error']).to eq('Some students do not belong to the specified class') + end + + it 'returns an error if any permission form is not found' do + post :bulk_update, params: { + class_id: clazz.id, + student_ids: [student.id], + add_permission_form_ids: [0] # Invalid ID + } + + expect(response).to have_http_status(:not_found) + end + end + + describe 'when user is a project admin' do before do sign_in project_admin project_admin.add_role_for_project('admin', project) end it 'GET index returns permission forms from the project' do - Portal::PermissionForm.create!(name: 'Test Form 1', url: 'http://example1.com', project_id: project.id) - Portal::PermissionForm.create!(name: 'Test Form 2', url: 'http://example2.com', project_id: another_project.id) get :index expect(response).to have_http_status(:ok) expect(JSON.parse(response.body).size).to eq(1) expect(JSON.parse(response.body)).to match([ - hash_including('name' => 'Test Form 1', 'url' => 'http://example1.com', 'project_id' => project.id, 'is_archived' => false, 'can_delete' => true) + hash_including('name' => 'Test Form', 'url' => 'http://example.com', 'project_id' => project.id, 'is_archived' => false, 'can_delete' => true) ]) end it 'DELETE is allowed if the permission form belongs to the project' do - permission_form = Portal::PermissionForm.create!(name: 'Test Form', url: 'http://example.com', project_id: project.id) - expect(Portal::PermissionForm.count).to eq(1) + expect(Portal::PermissionForm.count).to eq(2) delete :destroy, params: { id: permission_form.id } expect(response).to have_http_status(:ok) - expect(Portal::PermissionForm.count).to eq(0) + expect(Portal::PermissionForm.count).to eq(1) end it 'DELETE is not allowed if the permission form does not belongs to the project' do - permission_form = Portal::PermissionForm.create!(name: 'Test Form', url: 'http://example.com', project_id: another_project.id ) - expect(Portal::PermissionForm.count).to eq(1) - delete :destroy, params: { id: permission_form.id } + expect(Portal::PermissionForm.count).to eq(2) + delete :destroy, params: { id: another_permission_form.id } expect(response).to have_http_status(:forbidden) - expect(Portal::PermissionForm.count).to eq(1) + expect(Portal::PermissionForm.count).to eq(2) end it 'GET search_teachers returns a list of teachers from the project' do - teacher1 = FactoryBot.create(:teacher, user: FactoryBot.create(:user, login: 'test_teacher1')) - cohort = FactoryBot.create(:admin_cohort, project: project) - teacher1.cohorts << cohort - teacher2 = FactoryBot.create(:teacher, user: FactoryBot.create(:user, login: 'test_teacher2')) get :search_teachers, params: { name: 'test_teacher' } @@ -156,15 +226,52 @@ body = JSON.parse(response.body) expect(body["teachers"].size).to eq(1) expect(body["teachers"]).to match([ - hash_including('id' => teacher1.id) + hash_including('id' => teacher.id) ]) end + + it 'GET projects returns a list of projects the user can manage permission forms for' do + get :projects + expect(response).to have_http_status(:ok) + expect(JSON.parse(response.body).size).to eq(1) + expect(JSON.parse(response.body)).to match([ + hash_including('id' => project.id) + ]) + end + + it 'GET class_permission_forms is allowed, but it limits permission forms to the project' do + get :class_permission_forms, params: { class_id: clazz.id } + expect(response).to have_http_status(:ok) + expect(JSON.parse(response.body).first['permission_forms'].size).to eq(1) + expect(JSON.parse(response.body).first['permission_forms'].first['id']).to eq(permission_form.id) + end + + it 'POST bulk_update is allowed' do + new_permission_form = FactoryBot.create(:permission_form, project: project) + + post :bulk_update, params: { + class_id: clazz.id, + student_ids: [student.id], + add_permission_form_ids: [new_permission_form.id], + remove_permission_form_ids: [] + } + expect(response).to have_http_status(:ok) + expect(student.reload.permission_forms).to include(new_permission_form) + end + + it 'POST bulk_update is not allowed if the permission forms do not belong to the project' do + post :bulk_update, params: { + class_id: clazz.id, + student_ids: [student.id], + add_permission_form_ids: [], + remove_permission_form_ids: [another_permission_form.id] + } + expect(response).to have_http_status(:forbidden) + expect(student.reload.permission_forms).to include(another_permission_form) + end end describe 'when user is a project researcher without access to manage permission forms' do - let (:project) { FactoryBot.create(:project) } - let (:project_researcher) { FactoryBot.generate(:author_user) } - before do sign_in project_researcher project_researcher.add_role_for_project('researcher', project, can_manage_permission_forms: false) @@ -177,31 +284,41 @@ end it 'DELETE is not allowed' do - permission_form = Portal::PermissionForm.create!(name: 'Test Form', url: 'http://example.com', project_id: project.id) - expect(Portal::PermissionForm.count).to eq(1) + expect(Portal::PermissionForm.count).to eq(2) delete :destroy, params: { id: permission_form.id } expect(response).to have_http_status(:forbidden) - expect(Portal::PermissionForm.count).to eq(1) + expect(Portal::PermissionForm.count).to eq(2) end it 'GET search_teachers is not allowed' do - teacher1 = FactoryBot.create(:teacher, user: FactoryBot.create(:user, login: 'test_teacher1')) - cohort = FactoryBot.create(:admin_cohort, project: project) - teacher1.cohorts << cohort + get :search_teachers, params: { name: 'test_teacher' } - teacher2 = FactoryBot.create(:teacher, user: FactoryBot.create(:user, login: 'test_teacher2')) + expect(response).to have_http_status(:forbidden) + end - get :search_teachers, params: { name: 'test_teacher' } + it 'GET projects is not allowed' do + get :projects + expect(response).to have_http_status(:forbidden) + end + + it 'GET class_permission_forms is not allowed' do + get :class_permission_forms, params: { class_id: clazz.id } + expect(response).to have_http_status(:forbidden) + end + it 'POST bulk_update is not allowed' do + new_permission_form = FactoryBot.create(:permission_form) + post :bulk_update, params: { + class_id: clazz.id, + student_ids: [student.id], + add_permission_form_ids: [new_permission_form.id], + remove_permission_form_ids: [] + } expect(response).to have_http_status(:forbidden) end end describe 'when user is a project researcher with access to manage permission forms' do - let (:project) { FactoryBot.create(:project) } - let (:another_project) { FactoryBot.create(:project) } - let (:project_researcher) { FactoryBot.generate(:author_user) } - before do sign_in project_researcher project_researcher.add_role_for_project('researcher', project, can_manage_permission_forms: true) @@ -209,41 +326,70 @@ end it 'GET index returns permission forms from the project' do - Portal::PermissionForm.create!(name: 'Test Form 1', url: 'http://example1.com', project_id: project.id) - Portal::PermissionForm.create!(name: 'Test Form 2', url: 'http://example2.com', project_id: another_project.id) get :index expect(response).to have_http_status(:ok) expect(JSON.parse(response.body).size).to eq(1) # Note that researchers are never allowed to delete a permission form expect(JSON.parse(response.body)).to match([ - hash_including('name' => 'Test Form 1', 'url' => 'http://example1.com', 'project_id' => project.id, 'is_archived' => false, 'can_delete' => false) + hash_including('name' => 'Test Form', 'url' => 'http://example.com', 'project_id' => project.id, 'is_archived' => false, 'can_delete' => false) ]) end it 'DELETE is not allowed' do - permission_form = Portal::PermissionForm.create!(name: 'Test Form', url: 'http://example.com', project_id: project.id) - expect(Portal::PermissionForm.count).to eq(1) + expect(Portal::PermissionForm.count).to eq(2) delete :destroy, params: { id: permission_form.id } expect(response).to have_http_status(:forbidden) - expect(Portal::PermissionForm.count).to eq(1) + expect(Portal::PermissionForm.count).to eq(2) end it 'GET search_teachers returns a list of teachers from the project' do - teacher1 = FactoryBot.create(:teacher, user: FactoryBot.create(:user, login: 'test_teacher1')) - cohort = FactoryBot.create(:admin_cohort, project: project) - teacher1.cohorts << cohort - - teacher2 = FactoryBot.create(:teacher, user: FactoryBot.create(:user, login: 'test_teacher2')) - get :search_teachers, params: { name: 'test_teacher' } expect(response).to have_http_status(:ok) body = JSON.parse(response.body) expect(body["teachers"].size).to eq(1) expect(body["teachers"]).to match([ - hash_including('id' => teacher1.id) + hash_including('id' => teacher.id) ]) end - end + it 'GET projects returns a list of projects the user can manage permission forms for' do + get :projects + expect(response).to have_http_status(:ok) + expect(JSON.parse(response.body).size).to eq(1) + expect(JSON.parse(response.body)).to match([ + hash_including('id' => project.id) + ]) + end + + it 'GET class_permission_forms is allowed, but it limits permission forms to the project' do + get :class_permission_forms, params: { class_id: clazz.id } + expect(response).to have_http_status(:ok) + expect(JSON.parse(response.body).first['permission_forms'].size).to eq(1) + expect(JSON.parse(response.body).first['permission_forms'].first['id']).to eq(permission_form.id) + end + + it 'POST bulk_update is allowed' do + new_permission_form = FactoryBot.create(:permission_form, project: project) + post :bulk_update, params: { + class_id: clazz.id, + student_ids: [student.id], + add_permission_form_ids: [new_permission_form.id], + remove_permission_form_ids: [] + } + expect(response).to have_http_status(:ok) + expect(student.reload.permission_forms).to include(new_permission_form) + end + + it 'POST bulk_update is not allowed if the permission forms do not belong to the project' do + post :bulk_update, params: { + class_id: clazz.id, + student_ids: [student.id], + add_permission_form_ids: [], + remove_permission_form_ids: [another_permission_form.id] + } + expect(response).to have_http_status(:forbidden) + expect(student.reload.permission_forms).to include(another_permission_form) + end + end end diff --git a/rails/spec/models/user_spec.rb b/rails/spec/models/user_spec.rb index 9da7ba7b7..4cd32d499 100644 --- a/rails/spec/models/user_spec.rb +++ b/rails/spec/models/user_spec.rb @@ -1181,6 +1181,34 @@ def create_user(options = {}) } it { is_expected.to be false} end + + context "when check_can_manage_permission_forms is true" do + subject { user.is_researcher_for_clazz?(clazz, check_can_manage_permission_forms: true) } + + context "when the user is not a researcher for the class" do + it { is_expected.to be false} + end + + context "when the user is a researcher for the class but cannot manage permission forms" do + let(:user) { + researcher = FactoryBot.generate(:researcher_user) + researcher.researcher_for_projects << project + researcher + } + it { is_expected.to be false} + end + + context "when the user is a researcher for the class and can manage permission forms" do + let(:user) { + researcher = FactoryBot.generate(:researcher_user) + researcher.researcher_for_projects << project + researcher.add_role_for_project('researcher', project, can_manage_permission_forms: true) + researcher + } + it { is_expected.to be true} + end + end + end # TODO: auto-generated