diff --git a/app/controllers/course/gradebook_controller.rb b/app/controllers/course/gradebook_controller.rb index 64e2c66989..fa945b42bf 100644 --- a/app/controllers/course/gradebook_controller.rb +++ b/app/controllers/course/gradebook_controller.rb @@ -24,7 +24,7 @@ def index def update_weights authorize! :manage_gradebook_weights, current_course - updates = update_weights_params[:weights].map { |entry| parse_weight_entry(entry) } + updates = (update_weights_params[:weights] || []).map { |entry| parse_weight_entry(entry) } Course::Gradebook::Contribution.bulk_update(course: current_course, updates: updates) render json: { weights: serialize_weight_updates(updates) } rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotFound => e diff --git a/app/models/components/course/gradebook_ability_component.rb b/app/models/components/course/gradebook_ability_component.rb index d54a56cca6..6b2221947d 100644 --- a/app/models/components/course/gradebook_ability_component.rb +++ b/app/models/components/course/gradebook_ability_component.rb @@ -3,7 +3,7 @@ module Course::GradebookAbilityComponent include AbilityHost::Component def define_permissions - can :read_gradebook, Course, id: course.id if course_user&.staff? + can :read_gradebook, Course, id: course.id if course_user&.manager_or_owner? can :manage_gradebook_weights, Course, id: course.id if course_user&.manager_or_owner? can :manage_gradebook_settings, Course, id: course.id if course_user&.manager_or_owner? super diff --git a/spec/controllers/course/gradebook_controller_spec.rb b/spec/controllers/course/gradebook_controller_spec.rb index a60c3d91df..021939f00c 100644 --- a/spec/controllers/course/gradebook_controller_spec.rb +++ b/spec/controllers/course/gradebook_controller_spec.rb @@ -7,17 +7,17 @@ with_tenant(:instance) do let(:course) { create(:course) } let(:student) { create(:course_user, :student, course: course) } - let(:staff) { create(:course_user, :teaching_assistant, course: course) } + let(:manager) { create(:course_user, :manager, course: course) } describe '#index' do render_views subject { get :index, params: { course_id: course.id }, format: :json } context 'when the gradebook component is disabled' do - let(:ta) { create(:course_teaching_assistant, course: course) } + let(:owner) { create(:course_owner, course: course) } before do - controller_sign_in(controller, ta.user) + controller_sign_in(controller, owner.user) allow(controller).to receive_message_chain('current_component_host.[]').and_return(nil) end @@ -26,6 +26,20 @@ end end + context 'when a owner visits the page' do + let(:owner) { create(:course_owner, course: course) } + before { controller_sign_in(controller, owner.user) } + + it { expect(subject).to be_successful } + end + + context 'when a manager visits the page' do + let(:manager) { create(:course_manager, course: course) } + before { controller_sign_in(controller, manager.user) } + + it { expect(subject).to be_successful } + end + context 'when a student visits the page' do let(:student) { create(:course_student, course: course) } before { controller_sign_in(controller, student.user) } @@ -37,33 +51,23 @@ let(:ta) { create(:course_teaching_assistant, course: course) } before { controller_sign_in(controller, ta.user) } - it { expect(subject).to be_successful } - - it 'returns all required top-level keys' do - subject - data = JSON.parse(response.body) - %w[categories tabs assessments students submissions].each do |key| - expect(data).to have_key(key), "expected response to have key '#{key}'" - end + it 'is denied' do + expect { get :index, params: { course_id: course.id }, format: :json }. + to raise_error(CanCan::AccessDenied) end end - context 'when a manager visits the page' do - let(:manager) { create(:course_manager, course: course) } - before { controller_sign_in(controller, manager.user) } - - it { expect(subject).to be_successful } - end - context 'when an observer visits the page' do let(:observer) { create(:course_observer, course: course) } before { controller_sign_in(controller, observer.user) } - it { expect(subject).to be_successful } + it 'is denied' do + expect { get :index, params: { course_id: course.id }, format: :json }. + to raise_error(CanCan::AccessDenied) + end end context 'with a published assessment and a graded submission' do - let(:ta) { create(:course_teaching_assistant, course: course) } let(:tab) { course.assessment_categories.first.tabs.first } let!(:assessment) do create(:course_assessment_assessment, :published_with_mcq_question, @@ -77,7 +81,7 @@ before do submission.answers.update_all(grade: 5.0, current_answer: true) - controller_sign_in(controller, ta.user) + controller_sign_in(controller, manager.user) end it 'includes the assessment in the assessments array' do @@ -129,9 +133,8 @@ end context 'when a student has an external ID' do - let(:ta) { create(:course_teaching_assistant, course: course) } let!(:student) { create(:course_student, course: course, external_id: 'EXT-123') } - before { controller_sign_in(controller, ta.user) } + before { controller_sign_in(controller, manager.user) } it 'returns the external ID in the students array' do subject @@ -142,8 +145,50 @@ end end - context 'with a graded submission where the answer grade is exactly 0' do + context 'when a phantom student is enrolled' do + let!(:real_student) { create(:course_student, course: course) } + let!(:phantom_student) { create(:course_student, :phantom, course: course) } + before { controller_sign_in(controller, manager.user) } + + it 'omits phantom users from the students array' do + subject + data = JSON.parse(response.body) + ids = data['students'].map { |s| s['id'] } + expect(ids).to include(real_student.user_id) + expect(ids).not_to include(phantom_student.user_id) + end + end + + context 'when the course has no published assessments or students' do + let(:ta) { create(:course_teaching_assistant, course: course) } + before { controller_sign_in(controller, manager.user) } + + it 'returns empty assessments and students without error' do + subject + expect(response).to be_successful + data = JSON.parse(response.body) + expect(data['assessments']).to eq([]) + expect(data['students']).to eq([]) + expect(data['submissions']).to eq([]) + end + end + + context 'with an unpublished (draft) assessment' do let(:ta) { create(:course_teaching_assistant, course: course) } + let(:tab) { course.assessment_categories.first.tabs.first } + let!(:draft) do + create(:course_assessment_assessment, course: course, tab: tab, published: false) + end + before { controller_sign_in(controller, manager.user) } + + it 'excludes the draft from the assessments array' do + subject + data = JSON.parse(response.body) + expect(data['assessments'].map { |a| a['id'] }).not_to include(draft.id) + end + end + + context 'with a graded submission where the answer grade is exactly 0' do let(:tab) { course.assessment_categories.first.tabs.first } let!(:assessment) do create(:course_assessment_assessment, :published_with_mcq_question, @@ -157,7 +202,7 @@ before do submission.answers.update_all(grade: 0.0, current_answer: true) - controller_sign_in(controller, ta.user) + controller_sign_in(controller, manager.user) end it 'returns grade 0 (not null) in the submissions array' do @@ -172,7 +217,6 @@ end context 'with a graded submission where answer grades are null (blank)' do - let(:ta) { create(:course_teaching_assistant, course: course) } let(:tab) { course.assessment_categories.first.tabs.first } let!(:assessment) do create(:course_assessment_assessment, :published_with_mcq_question, @@ -186,7 +230,7 @@ before do submission.answers.update_all(grade: nil, current_answer: true) - controller_sign_in(controller, ta.user) + controller_sign_in(controller, manager.user) end it 'returns null grade (not 0) in the submissions array' do @@ -202,7 +246,6 @@ context 'when the course has an external assessment' do render_views - let(:ta) { create(:course_teaching_assistant, course: course) } let(:gb_student) { create(:course_student, course: course) } let!(:external) do create(:course_external_assessment, course: course, title: 'Midterm', maximum_grade: 50) @@ -211,7 +254,7 @@ create(:course_external_assessment_grade, external_assessment: external, course_user: gb_student, grade: 41) end - before { controller_sign_in(controller, ta.user) } + before { controller_sign_in(controller, manager.user) } it 'merges the external into assessments with a negative id and external flag' do subject @@ -232,30 +275,20 @@ expect(sub['studentId']).to eq(gb_student.user_id) expect(sub['grade']).to eq(41.0) end + end - it 'emits a synthetic External Assessments category' do - subject - data = JSON.parse(response.body) - cat = data['categories'].find { |c| c['id'] == Course::ExternalAssessment::SYNTHETIC_CATEGORY_ID } - expect(cat).to be_present - expect(cat['title']).to eq('External Assessments') + context 'when an external assessment has no grades' do + render_views + let!(:external) do + create(:course_external_assessment, course: course, title: 'Ungraded', maximum_grade: 20) end + before { controller_sign_in(controller, manager.user) } - it 'emits a synthetic tab with negative id under the synthetic category' do + it 'still emits the external leaf but no submission row for it' do subject data = JSON.parse(response.body) - tab = data['tabs'].find { |t| t['id'] == external.synthetic_tab_id } - expect(tab).to be_present - expect(tab['categoryId']).to eq(Course::ExternalAssessment::SYNTHETIC_CATEGORY_ID) - end - - it 'creates no real tab or category for the external' do - tab_count_before = Course::Assessment::Tab.count - cat_count_before = Course::Assessment::Category.count - subject - expect(Course::Assessment::Tab.count).to eq(tab_count_before) - expect(Course::Assessment::Category.count).to eq(cat_count_before) - expect(Course::Assessment::Category.where(title: 'External Assessments')).to be_empty + expect(data['assessments'].map { |a| a['id'] }).to include(-external.id) + expect(data['submissions'].select { |s| s['assessmentId'] == -external.id }).to eq([]) end end end @@ -267,13 +300,12 @@ Course::ExternalAssessment.create_for_course!(course: course, title: 'Midterm', maximum_grade: 50.0, weight: 40) end - let(:ta) { create(:course_teaching_assistant, course: course) } before do ctx = Struct.new(:current_course, :key).new(course, Course::GradebookComponent.key) Course::Settings::GradebookComponent.new(ctx).weighted_view_enabled = true course.save! - controller_sign_in(controller, ta.user) + controller_sign_in(controller, manager.user) end subject(:body) do @@ -310,9 +342,7 @@ end describe 'PATCH update_weights' do - let(:manager) { create(:course_manager, course: course) } let(:ta) { create(:course_teaching_assistant, course: course) } - let(:student) { create(:course_student, course: course) } let(:category) { create(:course_assessment_category, course: course) } let!(:tab1) { create(:course_assessment_tab, category: category) } let!(:tab2) { create(:course_assessment_tab, category: category) } @@ -334,6 +364,21 @@ def weight_for(tab) expect(weight_for(tab2)).to eq(40) end + it 'accepts an empty weights array as a no-op' do + patch :update_weights, params: { course_id: course.id, weights: [] }, format: :json + expect(response).to have_http_status(:ok) + expect(JSON.parse(response.body)['weights']).to eq([]) + end + + it 'ignores keys outside the permitted weights schema' do + patch :update_weights, + params: { course_id: course.id, + weights: [tabId: tab1.id, weight: 60, bogus: 'x'] }, + format: :json + expect(response).to have_http_status(:ok) + expect(weight_for(tab1)).to eq(60) + end + it 'accepts sum < 100' do patch :update_weights, params: { course_id: course.id, weights: [tabId: tab1.id, weight: 30] }, @@ -462,13 +507,22 @@ def weight_for(tab) entry = JSON.parse(response.body)['weights'].first expect(entry['excludedAssessmentIds']).to eq([a1.id]) end + + it 'omits assessmentWeights from the echoed entry in equal mode' do + post :update_weights, as: :json, params: { + course_id: course.id, + weights: [tabId: tab.id, weight: '50', weightMode: 'equal'] + } + expect(response).to have_http_status(:ok) + entry = JSON.parse(response.body)['weights'].first + expect(entry).not_to have_key('assessmentWeights') + expect(entry['weightMode']).to eq('equal') + end end end describe 'GET index — weighted view fields' do render_views - let(:manager) { create(:course_manager, course: course) } - let(:ta) { create(:course_teaching_assistant, course: course) } let(:category) { create(:course_assessment_category, course: course) } let!(:tab) { create(:course_assessment_tab, category: category) } let!(:contribution) { create(:course_gradebook_contribution, tab: tab, course: course, weight: 30) } @@ -506,13 +560,6 @@ def weight_for(tab) expect(tab_json['gradebookWeight']).to eq(30) end - it 'returns canManageWeights false for TA' do - controller_sign_in(controller, ta.user) - get :index, params: { course_id: course.id }, format: :json - body = JSON.parse(response.body) - expect(body['canManageWeights']).to eq(false) - end - it 'serializes weightMode on tabs and gradebookWeight on assessments when weighted view is enabled' do controller_sign_in(controller, manager.user) get :index, params: { course_id: course.id }, format: :json @@ -534,7 +581,6 @@ def weight_for(tab) describe 'external assessment ordering' do render_views - let(:manager) { create(:course_manager, course: course) } it 'serializes externals in position order, not creation order' do first = create(:course_external_assessment, course: course, title: 'Zeta') diff --git a/spec/models/course/gradebook_ability_spec.rb b/spec/models/course/gradebook_ability_spec.rb index 7247c2084c..8012d3c56b 100644 --- a/spec/models/course/gradebook_ability_spec.rb +++ b/spec/models/course/gradebook_ability_spec.rb @@ -13,6 +13,10 @@ it { is_expected.to be_able_to(:read_gradebook, course) } it { is_expected.to be_able_to(:manage_gradebook_weights, course) } it { is_expected.to be_able_to(:manage_gradebook_settings, course) } + it 'cannot read the gradebook of a different course' do + other_course = create(:course) + expect(subject).not_to be_able_to(:read_gradebook, other_course) + end end context 'when the user is a Course Owner' do @@ -21,12 +25,16 @@ it { is_expected.to be_able_to(:read_gradebook, course) } it { is_expected.to be_able_to(:manage_gradebook_weights, course) } it { is_expected.to be_able_to(:manage_gradebook_settings, course) } + it 'cannot read the gradebook of a different course' do + other_course = create(:course) + expect(subject).not_to be_able_to(:read_gradebook, other_course) + end end context 'when the user is a Teaching Assistant' do let(:course_user) { create(:course_teaching_assistant, course: course) } let(:user) { course_user.user } - it { is_expected.to be_able_to(:read_gradebook, course) } + it { is_expected.not_to be_able_to(:read_gradebook, course) } it { is_expected.not_to be_able_to(:manage_gradebook_weights, course) } it { is_expected.not_to be_able_to(:manage_gradebook_settings, course) } end @@ -38,5 +46,13 @@ it { is_expected.not_to be_able_to(:manage_gradebook_weights, course) } it { is_expected.not_to be_able_to(:manage_gradebook_settings, course) } end + + context 'when the user is a Course Observer' do + let(:course_user) { create(:course_observer, course: course) } + let(:user) { course_user.user } + it { is_expected.not_to be_able_to(:read_gradebook, course) } + it { is_expected.not_to be_able_to(:manage_gradebook_weights, course) } + it { is_expected.not_to be_able_to(:manage_gradebook_settings, course) } + end end end