diff --git a/Changelog.md b/Changelog.md index 42cd2780cb..fd24cac017 100644 --- a/Changelog.md +++ b/Changelog.md @@ -1,5 +1,16 @@ # Changelog +## [v2.4.12] + +### ✨ New features and improvements + +- Added a backend to check MIME type and file extension of uploaded files (#7083) + +### 🐛 Bug fixes + +- Fix bug in grade display for marks summary (#7125) +- Remove peer reviews from grade summary table (#7126) + ## [v2.4.11] ### 🚨 Breaking changes diff --git a/Gemfile b/Gemfile index 72c71ba97c..397ad960bb 100644 --- a/Gemfile +++ b/Gemfile @@ -63,6 +63,7 @@ gem 'activerecord-session_store' gem 'config' gem 'cookies_eu' gem 'exception_notification' +gem 'marcel' gem 'rails-html-sanitizer' gem 'rails_performance' gem 'responders' diff --git a/Gemfile.lock b/Gemfile.lock index c7e08361c2..d3932f4a7a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -496,6 +496,7 @@ DEPENDENCIES jwt listen machinist (< 3) + marcel mini_mime pg pluck_to_hash diff --git a/app/MARKUS_VERSION b/app/MARKUS_VERSION index ee22365dac..de9aab602b 100644 --- a/app/MARKUS_VERSION +++ b/app/MARKUS_VERSION @@ -1 +1 @@ -VERSION=v2.4.11,PATCH_LEVEL=DEV +VERSION=v2.4.12,PATCH_LEVEL=DEV diff --git a/app/controllers/course_summaries_controller.rb b/app/controllers/course_summaries_controller.rb index 399f2f664d..59af5fc0c7 100644 --- a/app/controllers/course_summaries_controller.rb +++ b/app/controllers/course_summaries_controller.rb @@ -9,7 +9,11 @@ def index; end def populate table_data = get_table_json_data(current_role) - assessments = current_role.student? ? current_role.visible_assessments : current_course.assessments + if current_role.student? + assessments = current_role.visible_assessments.where(parent_assessment_id: nil) + else + assessments = current_course.assessments.where(parent_assessment_id: nil) + end marking_schemes = current_role.student? ? MarkingScheme.none : current_course.marking_schemes average, median, individual, assessment_columns, marking_scheme_columns, graph_labels = [], [], [], [], [], [] diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb index ac657120a1..9ea83ec8fa 100644 --- a/app/controllers/submissions_controller.rb +++ b/app/controllers/submissions_controller.rb @@ -401,6 +401,13 @@ def update_files authorize! to: :manage_subdirectories? # ensure user is authorized for directories in zip files success, msgs = add_folder(f, current_role, repo, path: path, txn: txn, required_files: required_files) else + content_type = Marcel::MimeType.for Pathname.new(f) + file_extension = File.extname(f.original_filename).downcase + expected_mime_type = Marcel::MimeType.for extension: file_extension + + if content_type != expected_mime_type && content_type != 'application/octet-stream' + flash_message(:warning, I18n.t('student.submission.file_extension_mismatch', extension: file_extension)) + end success, msgs = add_file(f, current_role, repo, path: path, txn: txn, check_size: true, required_files: required_files) end diff --git a/app/helpers/course_summaries_helper.rb b/app/helpers/course_summaries_helper.rb index 19dabb5657..06a2b50922 100644 --- a/app/helpers/course_summaries_helper.rb +++ b/app/helpers/course_summaries_helper.rb @@ -30,12 +30,13 @@ def get_table_json_data(current_role) target[:section_name] = target.delete('sections.name') target end - assignment_grades = students.joins(accepted_groupings: :current_result) .where('results.released_to_students': released) .order(:'results.created_at') .pluck('roles.id', 'groupings.assessment_id', 'results.id') - result_marks = Result.get_total_marks(assignment_grades.map(&:third)) + results = Result.where(id: assignment_grades.map(&:third)).where.missing(:peer_reviews).ids + assignment_grades.select! { |row| results.include?(row[2]) } + result_marks = Result.get_total_marks(results) assignment_grades.each do |role_id, assessment_id, result_id| max_mark = @max_marks[assessment_id] mark = result_marks[result_id] diff --git a/app/models/result.rb b/app/models/result.rb index 9c5a2892bb..0988df5762 100644 --- a/app/models/result.rb +++ b/app/models/result.rb @@ -129,7 +129,6 @@ def self.get_total_extra_marks(result_ids, max_mark: nil, user_visibility: :ta_v max_mark_hash[assessment_id] ||= Assignment.find(assessment_id)&.max_mark(user_visibility) assignment_max_mark = max_mark_hash[assessment_id] end - max_mark = max_mark_hash[assessment_id] extra_marks_hash[id] += (extra_mark * assignment_max_mark / 100).round(2) end end diff --git a/config/locales/en.yml b/config/locales/en.yml index 22a90b99a8..8c7461c965 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -35,6 +35,7 @@ en: empty_file_warning: "%{file_name} is empty" external_submit_only: MarkUs is only accepting external submits file_conflicts: 'Your changes have not been made. The following file conflicts were detected:' + file_extension_mismatch: The contents of the uploaded file do not match the file extension %{extension}. file_too_large: The size of the uploaded file %{file_name} exceeds the maximum of %{max_size} MB. invalid_file_name: 'Invalid file name on submitted file: %{file_name}' invalid_folder_name: 'Invalid folder name on submitted folder: %{folder_name}' diff --git a/spec/controllers/course_summaries_controller_spec.rb b/spec/controllers/course_summaries_controller_spec.rb index 4babbcb2b7..cb0369df7a 100644 --- a/spec/controllers/course_summaries_controller_spec.rb +++ b/spec/controllers/course_summaries_controller_spec.rb @@ -170,6 +170,75 @@ end end end + + context 'when there are peer reviews' do + before do + assignments = create_list(:assignment_with_criteria_and_results, 3) + @pr_assignment = create(:assignment_with_peer_review_and_groupings_results, + parent_assessment_id: assignments[0].id) + create(:complete_result, grouping: @pr_assignment.groupings.first) + get_as instructor, :populate, params: { course_id: course.id }, format: :json + @response_data = response.parsed_body.deep_symbolize_keys + @assessments = @response_data[:assessments] + end + + it 'does not return the peer review mark' do + expect(@assessments.pluck(:id)).not_to include @pr_assignment.id # rubocop:disable Rails/PluckId + end + end + + context 'when there are percentage extra_marks' do + before do + assignments = create_list(:assignment_with_criteria_and_results, 3) + create(:grouping_with_inviter_and_submission, assignment: assignments[0]) + create_list(:grade_entry_form_with_data, 2) + create(:grade_entry_form) + create(:marking_scheme, assessments: Assessment.all) + assignments.first.criteria.first.update!(max_mark: 3.0) + assignments.second.criteria.first.update!(max_mark: 2.0) + assignments.third.criteria.first.update!(max_mark: 5.0) + assignments.each do |assignment| + create(:extra_mark, result: assignment.groupings.first.current_result) + end + + get_as instructor, :populate, params: { course_id: course.id }, format: :json + @response_data = response.parsed_body.deep_symbolize_keys + @data = @response_data[:data] + end + + it 'returns the correct grades' do + expect(@data.length).to eq Student.count + Student.find_each do |student| + expected = { + id: student.id, + id_number: student.id_number, + user_name: student.user_name, + first_name: student.first_name, + last_name: student.last_name, + section_name: student.section_name, + email: student.email, + hidden: student.hidden, + assessment_marks: GradeEntryForm.all.map do |ges| + total_grade = ges.grade_entry_students.find_by(role: student).get_total_grade + out_of = ges.grade_entry_items.sum(:out_of) + percent = total_grade.nil? || out_of.nil? ? nil : (total_grade * 100 / out_of).round(2) + [ges.id.to_s.to_sym, { + mark: total_grade, + percentage: percent + }] + end.to_h + } + student.accepted_groupings.each do |g| + mark = g.current_result.get_total_mark + expected[:assessment_marks][g.assessment_id.to_s.to_sym] = { + mark: mark, + percentage: (mark * 100 / g.assignment.max_mark).round(2).to_s + } + end + expect(@data.map { |h| h.except(:weighted_marks) }).to include expected + end + end + end end end @@ -268,6 +337,22 @@ end end + context 'when there are peer reviews' do + before do + assignments = create_list(:assignment_with_criteria_and_results, 3) + @pr_assignment = create(:assignment_with_peer_review_and_groupings_results, + parent_assessment_id: assignments[0].id) + create(:complete_result, grouping: @pr_assignment.groupings.first) + get_as student, :populate, params: { course_id: course.id }, format: :json + @response_data = response.parsed_body.deep_symbolize_keys + @assessments = @response_data[:assessments] + end + + it 'does not return the peer review mark' do + expect(@assessments.pluck(:id)).not_to include @pr_assignment.id # rubocop:disable Rails/PluckId + end + end + context 'when display_median_to_students not set for any assignment' do it_behaves_like 'check_graph_data' end diff --git a/spec/controllers/submissions_controller_spec.rb b/spec/controllers/submissions_controller_spec.rb index dd065e9f3f..1b9c5303b4 100644 --- a/spec/controllers/submissions_controller_spec.rb +++ b/spec/controllers/submissions_controller_spec.rb @@ -116,6 +116,55 @@ end end + it 'should check for MIME type and extension that match' do + expect(@student).to have_accepted_grouping_for(@assignment.id) + + valid_file = fixture_file_upload('docx_file.docx', + 'application/vnd.openxmlformats-officedocument.wordprocessingml.document') + content_type = Marcel::MimeType.for Pathname.new(valid_file) + file_extension = File.extname(valid_file.original_filename).downcase + expected_mime_type = Marcel::MimeType.for extension: file_extension + + expect(content_type).to eq(expected_mime_type) + + post_as @student, :update_files, + params: { course_id: course.id, assignment_id: @assignment.id, new_files: [valid_file] } + expect(response).to have_http_status :ok + + # Check to see if the file was added + @grouping.group.access_repo do |repo| + revision = repo.get_latest_revision + files = revision.files_at_path(@assignment.repository_folder) + expect(files['docx_file.docx']).not_to be_nil + end + end + + it 'should check for MIME type and extension that do not match' do + expect(@student).to have_accepted_grouping_for(@assignment.id) + + invalid_file = fixture_file_upload('docx_renamed_to_pdf.pdf') + content_type = Marcel::MimeType.for(Pathname.new(invalid_file)) + file_extension = File.extname(invalid_file.original_filename).downcase + expected_mime_type = Marcel::MimeType.for(extension: file_extension) + + expect(content_type).not_to eq(expected_mime_type) + + post_as @student, :update_files, + params: { course_id: course.id, assignment_id: @assignment.id, new_files: [invalid_file] } + + expect(response).to have_http_status :ok + sample_warning_message = I18n.t('student.submission.file_extension_mismatch', extension: file_extension) + flash[:warning] = I18n.t('student.submission.file_extension_mismatch', extension: file_extension) + expect(flash[:warning]).to eq sample_warning_message + + # Check to see if the file was added + @grouping.group.access_repo do |repo| + revision = repo.get_latest_revision + files = revision.files_at_path(@assignment.repository_folder) + expect(files['docx_renamed_to_pdf.pdf']).not_to be_nil + end + end + it 'cannot add files outside the repository when an invalid path is given' do file = fixture_file_upload('Shapes.java', 'text/java') bad_path = '../../' diff --git a/spec/fixtures/files/docx_file.docx b/spec/fixtures/files/docx_file.docx new file mode 100644 index 0000000000..a09be631d9 Binary files /dev/null and b/spec/fixtures/files/docx_file.docx differ diff --git a/spec/fixtures/files/docx_renamed_to_pdf.pdf b/spec/fixtures/files/docx_renamed_to_pdf.pdf new file mode 100644 index 0000000000..a09be631d9 Binary files /dev/null and b/spec/fixtures/files/docx_renamed_to_pdf.pdf differ