feat: grades upload flow by YheChen · Pull Request #8008 · MarkUsProject/Markus

YheChen · 2026-06-18T22:30:32Z

Proposed Changes

Adds CSV upload support for criterion marks from the assignment Grades tab.

This PR adds an Upload button for instructors on the assignment Grades tab. The upload flow accepts a CSV in the same general format as the existing Grades tab CSV export / grade entry form uploads, updates marks for matching assignment criteria, supports an overwrite option, and ignores total mark and bonus/deduction columns.

Also adds backend import handling, authorization, translations, and model/controller tests.

Fixes #7794

Screenshots of your changes (if applicable)

Upload Button

Upload Modal

Associated documentation repository pull request (if applicable)

MarkUsProject/Wiki#268

Type of Change

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)	X
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)	X
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

Before opening your pull request:

I have performed a self-review of my changes.
- Check that all changed files included in this pull request are intentional changes.
- Check that all changes are relevant to the purpose of this pull request, as described above.
I have added tests for my changes, if applicable.
- This is required for all bug fixes and new features.
I have updated the project documentation, if applicable.
- This is required for new features.
If this is my first contribution, I have added myself to the list of contributors.

After opening your pull request:

I have updated the project Changelog (this is required for all changes).
I have verified that the pre-commit.ci checks have passed.
I have verified that the CI tests have passed.
I have reviewed the test coverage changes reported by Coveralls.
I have requested a review from a project maintainer.

Questions and Comments

coveralls · 2026-06-18T22:48:49Z

Coverage Report for CI Build 28135968175

Coverage increased (+0.03%) to 90.215%

Details

Coverage increased (+0.03%) from the base build.
Patch coverage: 6 uncovered changes across 3 files (196 of 202 lines covered, 97.03%).
No coverage regressions found.

Uncovered Changes

File	Changed	Covered	%
app/models/assignment.rb	65	62	95.38%
app/javascript/Components/assignment_summary_table.jsx	3	1	33.33%
app/javascript/Components/Modals/assignment_grades_upload_modal.jsx	4	3	75.0%
Total (9 files)	202	196	97.03%

Coverage Regressions

No coverage regressions found.

Coverage Stats


Relevant Lines:	50815
Covered Lines:	46863
Line Coverage:	92.22%
Relevant Branches:	2388
Covered Branches:	1134
Branch Coverage:	47.49%
Branches in Coverage %:	Yes
Coverage Strength:	127.41 hits per line

💛 - Coveralls

david-yz-liu

@YheChen nice work, I left a few comments. Please also make sure to update the screenshots in the PR description and the documentation Wiki.

david-yz-liu · 2026-06-23T00:55:06Z

 class AssignmentPolicy < ApplicationPolicy
  default_rule :manage?
  alias_rule :summary?, to: :view?
+  alias_rule :upload_grades?, to: :manage?


This isn't necessary because of the default_rule :manage? call

david-yz-liu · 2026-06-23T01:13:27Z

+                            .joins(grouping: :group)
+                            .includes(:marks, grouping: :group)
+                            .index_by { |result| result.grouping.group.group_name }
+    results_by_user_name = current_results


Don't include this. The group name is required; the list of users is optional, and should be ignored if it's in the CSV upload. There is no need to have a fallback query (see my further comment below on result_for_uploaded_marks_row)

david-yz-liu · 2026-06-23T01:14:52Z

+
+      raise CsvInvalidLineError if criterion_columns == :invalid
+
+      result = result_for_uploaded_marks_row(row, group_name_index, user_name_index,


I wouldn't define a helper function here, it's not worth it. Do a lookup on row[group_name_index]. This will either be a Result or nil, and is the only thin you need to use.

david-yz-liu · 2026-06-23T01:17:16Z

+          mark = result.marks.find_or_initialize_by(criterion: criterion)
+          next if !overwrite && !mark.mark.nil?
+
+          mark_value = parse_uploaded_mark(row[column_index])


No need to make this a helper function, inline the logic here

david-yz-liu · 2026-06-23T01:20:28Z

+          next if !overwrite && !mark.mark.nil?
+
+          mark_value = parse_uploaded_mark(row[column_index])
+          unless mark.update(mark: mark_value,


Doing this one record at a time will be quite slow, even with the transaction. Modify the logic to accumulate a set of updates and use upsert_all

david-yz-liu · 2026-06-23T01:21:44Z

+      elsif criterion_columns.nil?
+        begin
+          criterion_columns = assignment_upload_criterion_columns(headers, row, criteria_by_name)
+        rescue CsvInvalidLineError


I'm not sure what the purpose of this exception handling and re-raising is

david-yz-liu · 2026-06-23T01:27:07Z

+        next
+      elsif criterion_columns.nil?
+        begin
+          criterion_columns = assignment_upload_criterion_columns(headers, row, criteria_by_name)


Similar to my above comment, I think this is overly complex and doesn't require a helper. Ignore the "totals" row; determine Criterion object based on the name, which is found only in the headers row.

You can ignore the "totals" row by skipping any row with a blank "group name" column, which I believe you already have some logic to do.

david-yz-liu · 2026-06-23T01:32:20Z

    criteria.exists? ? criteria.last.position + 1 : 1
  end

+  def criteria_by_upload_name


This helper function isn't necessary; again, inline the code above.

First, to create a hash use the pattern of

ta_criteria.map do |criteria| ... # end with an array of [key, value] end.to_h

Second, the most complex part of this logic is the conditional adding of "bonus". This appears elsewhere in the code for generating CSV files, so actually what I would suggest is to define a Criterion helper method called export_name that handles this logic, and then just call criterion.export_name here. Also modify the existing places in the CSV exporting to use this method.

The .to_s and strip calls are unnecessary. They are not found in the corresponding CSV export method.

david-yz-liu · 2026-06-23T01:34:49Z

    upload_file_requirement: Filename %{file_name} is not allowed for this assignment.
    upload_file_requirement_in_folder: Filename %{file_name} in %{file_path} is not allowed for this assignment.
+    upload_grades:
+      invalid_criteria: 'No matching criteria were found in the uploaded CSV. Unknown criteria: %{criteria}'


Reword this message to "The following criteria were not found for this assignment: %{criteria}"

david-yz-liu · 2026-06-23T19:34:33Z

      example_form_params[:assignment][:submission_rule_attributes][:periods_attributes] = submission_rule.id
      example_form_params
    end
+


Please revert the blank line changes in this file (here and below)

FYI I discovered that RuboCop had a rule RSpec/EmptyLineAfterFinalLet that was creating these blank lines, which I bypassed in my commit by doing
SKIP=rubocop git commit -m "Remove incidental controller spec blank lines"

david-yz-liu · 2026-06-23T19:37:42Z

+    criteria_by_name = ta_criteria.index_by(&:export_name)
+    ignored_headers = [Group.human_attribute_name(:group_name),
+                       I18n.t('results.total_mark'),
+                       'Bonus/Deductions'] +


Sorry I just noticed this; please internationalize this string (and replace here and in the CSV export)

david-yz-liu · 2026-06-23T19:45:20Z

+
+        if uploaded_criterion_columns.empty? || unknown_columns.present?
+          criterion_columns = []
+          raise CsvInvalidLineError,


Ah okay I understand a bit better now. When the criteria columns are invalid, we shouldn't raise a CsvInvalidLineError (which is handled by MarkusCsv.parse), but instead you can just raise the string directly. This will terminate this entire function; then, handle the error in the controller and report the specific error message to the user.

david-yz-liu · 2026-06-23T19:45:48Z

+        next
+      end
+
+      raise CsvInvalidLineError if criterion_columns.empty?


Doing the above comment will allow you to remove this check

david-yz-liu · 2026-06-23T19:52:03Z

+        else
+          new_mark_updates[key] = attrs.merge(created_at: now)
+        end
+        marks_by_result_and_criterion[key] = { id: mark&.[](:id), mark: attrs[:mark] }


I don't think this line has any effect (marks_by_result_and_criterion is not used further in this function)

david-yz-liu · 2026-06-23T19:55:51Z

+        if mark&.[](:id)
+          mark_updates[mark[:id]] = attrs.merge(id: mark[:id])
+        else
+          new_mark_updates[key] = attrs.merge(created_at: now)


Hmm I don't think the created_at timestamp is necessary? I thought this was address in Rails v7 (rails/rails#43003).

YheChen added 2 commits June 18, 2026 18:29

Add grades tab upload flow

ed2910b

changelog

815571f

Merge branch 'master' into feat/upload-assignment-grades

7325913

YheChen requested a review from david-yz-liu June 22, 2026 21:36

david-yz-liu reviewed Jun 23, 2026

View reviewed changes

YheChen changed the title ~~WIP: feat: grades upload flow~~ feat: grades upload flow Jun 23, 2026

YheChen added 3 commits June 23, 2026 12:31

addressed david changes I

9360d14

Merge branch 'master' into feat/upload-assignment-grades

f5640ae

addressed david changes II, updated tests

f18bdee

YheChen mentioned this pull request Jun 23, 2026

feat: added docs for uploading grades by CSV MarkUsProject/Wiki#268

Open

YheChen requested a review from david-yz-liu June 23, 2026 17:28

Merge branch 'master' into feat/upload-assignment-grades

56cd8ff

david-yz-liu reviewed Jun 23, 2026

View reviewed changes

YheChen added 3 commits June 24, 2026 18:27

Remove incidental controller spec blank lines

33e3667

other david changes

ff7834b

Remove incidental controller spec blank lines

6cf438e

YheChen requested a review from david-yz-liu June 24, 2026 23:30


		raise CsvInvalidLineError if criterion_columns == :invalid

		result = result_for_uploaded_marks_row(row, group_name_index, user_name_index,

Uh oh!

Conversation

YheChen commented Jun 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Upload Button

Upload Modal

Type of Change

Checklist

Questions and Comments

Uh oh!

coveralls commented Jun 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report for CI Build 28135968175

Coverage increased (+0.03%) to 90.215%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

david-yz-liu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YheChen Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

YheChen commented Jun 18, 2026 •

edited

Loading

coveralls commented Jun 18, 2026 •

edited

Loading

YheChen Jun 24, 2026 •

edited

Loading