diff --git a/app/controllers/course/gradebook_controller.rb b/app/controllers/course/gradebook_controller.rb index 2d1b7e761a..b0bc09bf7b 100644 --- a/app/controllers/course/gradebook_controller.rb +++ b/app/controllers/course/gradebook_controller.rb @@ -23,7 +23,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 7c6d0990d6..955608cada 100644 --- a/app/models/components/course/gradebook_ability_component.rb +++ b/app/models/components/course/gradebook_ability_component.rb @@ -3,10 +3,10 @@ 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? - can :grade, Course::ExternalAssessment if course_user&.teaching_staff? + can :grade, Course::ExternalAssessment if course_user&.manager_or_owner? super end end diff --git a/spec/controllers/course/external_assessments_controller_spec.rb b/spec/controllers/course/external_assessments_controller_spec.rb index 36fddb31b6..6123c2c34b 100644 --- a/spec/controllers/course/external_assessments_controller_spec.rb +++ b/spec/controllers/course/external_assessments_controller_spec.rb @@ -8,6 +8,7 @@ let(:course) { create(:course) } let(:manager) { create(:course_manager, course: course) } let(:ta) { create(:course_teaching_assistant, course: course) } + let(:observer) { create(:course_observer, course: course) } describe '#create' do render_views @@ -89,12 +90,24 @@ it 'returns 422 on a blank title' do post :create, params: params.merge(title: '') - expect(response).to have_http_status(:unprocessable_entity) + expect(response).to have_http_status(:unprocessable_content) end it 'returns 422 on a blank maximumGrade' do post :create, params: params.merge(maximumGrade: '') - expect(response).to have_http_status(:unprocessable_entity) + expect(response).to have_http_status(:unprocessable_content) + end + + it "serializes the tab weightMode as 'equal' when weighted" do + context = OpenStruct.new(current_course: course, key: Course::GradebookComponent.key) + Course::Settings::GradebookComponent.new(context).weighted_view_enabled = true + course.save! + + post :create, as: :json, params: { + course_id: course, title: 'Presentation', maximumGrade: 10, weight: 25 + } + json = JSON.parse(response.body) + expect(json['tab']['weightMode']).to eq('equal') end end @@ -105,11 +118,20 @@ expect { post :create, params: params }.to raise_error(CanCan::AccessDenied) end end + + context 'as an observer' do + before { controller_sign_in(controller, observer.user) } + + it 'is denied' do + expect { post :create, params: params }.to raise_error(CanCan::AccessDenied) + end + end end describe '#update' do render_views let!(:external) { create(:course_external_assessment, course: course, title: 'Mid') } + let(:gb_student) { create(:course_student, course: course) } context 'as a manager' do before { controller_sign_in(controller, manager.user) } @@ -146,10 +168,17 @@ expect(response).to have_http_status(:not_found) end + it 'returns 404 when grading an external that belongs to another course' do + other_external = create(:course_external_assessment) + put :grades, params: { course_id: course.id, id: other_external.id, format: :json, + studentId: gb_student.user_id, grade: 50 } + expect(response).to have_http_status(:not_found) + end + it 'returns 422 on a blank title' do patch :update, params: { course_id: course.id, id: external.id, format: :json, title: '' } - expect(response).to have_http_status(:unprocessable_entity) + expect(response).to have_http_status(:unprocessable_content) end it 'updates a bound flag' do @@ -185,6 +214,22 @@ expect(body['assessment']['gradebookWeight']).to eq(40.0) expect(body['tab']['gradebookWeight']).to eq(40.0) end + + it "serializes the tab weightMode as 'equal'" do + patch :update, as: :json, params: { + course_id: course, id: weighted_external.id, weight: 40 + } + body = JSON.parse(response.body) + expect(body['tab']['weightMode']).to eq('equal') + end + + it 'leaves the contribution weight untouched when no weight param is sent' do + patch :update, as: :json, params: { + course_id: course, id: weighted_external.id, title: 'Renamed' + } + expect(response).to be_successful + expect(weighted_external.gradebook_contribution.reload.weight).to eq(10) + end end it 'ignores a weight when weighted view is disabled' do @@ -199,6 +244,14 @@ expect(body['assessment']).not_to have_key('gradebookWeight') expect(body['tab']).not_to have_key('gradebookWeight') end + + it 'inserts a grade for a student who has none' do + expect do + put :grades, params: { course_id: course.id, id: external.id, format: :json, + studentId: gb_student.user_id, grade: 70 } + end.to change { Course::ExternalAssessmentGrade.count }.by(1) + expect(response).to be_successful + end end context 'as a teaching assistant' do @@ -251,8 +304,8 @@ let!(:external) { create(:course_external_assessment, course: course) } let(:gb_student) { create(:course_student, course: course) } - context 'as a teaching assistant (grading-capable staff)' do - before { controller_sign_in(controller, ta.user) } + context 'as a manager' do + before { controller_sign_in(controller, manager.user) } # The gradebook frontend keys students by user_id (json.studentId == course_user.user_id), # so #grades must resolve the course_user from the `studentId` param, not a course_user PK. @@ -302,9 +355,19 @@ end end - context 'as a student' do - let(:viewer) { create(:course_student, course: course) } - before { controller_sign_in(controller, viewer.user) } + context 'as a teaching assistant' do + before { controller_sign_in(controller, ta.user) } + + it 'is denied' do + expect do + put :grades, params: { course_id: course.id, id: external.id, format: :json, + studentId: gb_student.user_id, grade: 5 } + end.to raise_error(CanCan::AccessDenied) + end + end + + context 'as an observer' do + before { controller_sign_in(controller, observer.user) } it 'is denied' do expect do @@ -333,7 +396,7 @@ it 'rejects a payload whose id set does not match' do put :reorder, params: { course_id: course.id, format: :json, orderedIds: [a.id, b.id] } - expect(response).to have_http_status(:unprocessable_entity) + expect(response).to have_http_status(:unprocessable_content) end end end diff --git a/spec/controllers/course/gradebook_controller_spec.rb b/spec/controllers/course/gradebook_controller_spec.rb index 74d54d758f..2d29c26bf3 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] }, @@ -355,7 +400,7 @@ def weight_for(tab) params: { course_id: course.id, weights: [{ tabId: tab1.id, weight: 50 }, { tabId: tab2.id, weight: -1 }] }, format: :json - expect(response).to have_http_status(:unprocessable_entity) + expect(response).to have_http_status(:unprocessable_content) expect(weight_for(tab1)).to eq(10) end @@ -363,7 +408,7 @@ def weight_for(tab) patch :update_weights, params: { course_id: course.id, weights: [tabId: tab1.id, weight: 101] }, format: :json - expect(response).to have_http_status(:unprocessable_entity) + expect(response).to have_http_status(:unprocessable_content) end it 'rejects foreign tab id with 422' do @@ -373,7 +418,7 @@ def weight_for(tab) patch :update_weights, params: { course_id: course.id, weights: [tabId: other_tab.id, weight: 50] }, format: :json - expect(response).to have_http_status(:unprocessable_entity) + expect(response).to have_http_status(:unprocessable_content) end end @@ -445,7 +490,7 @@ def weight_for(tab) assessmentWeights: [assessmentId: a1.id, weight: '10'] ] } - expect(response).to have_http_status(:unprocessable_entity) + expect(response).to have_http_status(:unprocessable_content) end it 'persists and echoes per-assessment exclusion in equal mode' do @@ -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 @@ -531,5 +578,25 @@ def weight_for(tab) end end end + + describe 'external assessment ordering' do + render_views + + it 'serializes externals in position order, not creation order' do + first = create(:course_external_assessment, course: course, title: 'Zeta') + second = create(:course_external_assessment, course: course, title: 'Alpha') + # Make Alpha come first by position. + Course::ExternalAssessment.reorder!(course: course, ordered_ids: [second.id, first.id]) + + controller_sign_in(controller, manager.user) + get :index, params: { course_id: course.id, format: :json } + + body = JSON.parse(response.body) + external_titles = body['tabs']. + select { |t| t['categoryId'] == Course::ExternalAssessment::SYNTHETIC_CATEGORY_ID }. + map { |t| t['title'] } + expect(external_titles).to eq(%w[Alpha Zeta]) + end + end end end diff --git a/spec/models/course/gradebook_ability_spec.rb b/spec/models/course/gradebook_ability_spec.rb index 7247c2084c..7216922838 100644 --- a/spec/models/course/gradebook_ability_spec.rb +++ b/spec/models/course/gradebook_ability_spec.rb @@ -13,6 +13,11 @@ 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 { is_expected.to be_able_to(:grade, Course::ExternalAssessment) } + 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,14 +26,20 @@ 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 { is_expected.to be_able_to(:grade, Course::ExternalAssessment) } + 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) } + it { is_expected.not_to be_able_to(:grade, Course::ExternalAssessment) } end context 'when the user is a Course Student' do @@ -37,6 +48,16 @@ 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) } + it { is_expected.not_to be_able_to(:grade, Course::ExternalAssessment) } + 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) } + it { is_expected.not_to be_able_to(:grade, Course::ExternalAssessment) } end end end