WIP: feat: grades upload flow#8008
Conversation
Coverage Report for CI Build 27919845468Coverage increased (+0.03%) to 90.291%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
david-yz-liu
left a comment
david-yz-liu
left a comment
There was a problem hiding this comment.
@YheChen nice work, I left a few comments. Please also make sure to update the screenshots in the PR description and the documentation Wiki.
| class AssignmentPolicy < ApplicationPolicy | ||
| default_rule :manage? | ||
| alias_rule :summary?, to: :view? | ||
| alias_rule :upload_grades?, to: :manage? |
There was a problem hiding this comment.
This isn't necessary because of the default_rule :manage? call
| .joins(grouping: :group) | ||
| .includes(:marks, grouping: :group) | ||
| .index_by { |result| result.grouping.group.group_name } | ||
| results_by_user_name = current_results |
There was a problem hiding this comment.
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)
|
|
||
| raise CsvInvalidLineError if criterion_columns == :invalid | ||
|
|
||
| result = result_for_uploaded_marks_row(row, group_name_index, user_name_index, |
There was a problem hiding this comment.
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.
| mark = result.marks.find_or_initialize_by(criterion: criterion) | ||
| next if !overwrite && !mark.mark.nil? | ||
|
|
||
| mark_value = parse_uploaded_mark(row[column_index]) |
There was a problem hiding this comment.
No need to make this a helper function, inline the logic here
| next if !overwrite && !mark.mark.nil? | ||
|
|
||
| mark_value = parse_uploaded_mark(row[column_index]) | ||
| unless mark.update(mark: mark_value, |
There was a problem hiding this comment.
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
| elsif criterion_columns.nil? | ||
| begin | ||
| criterion_columns = assignment_upload_criterion_columns(headers, row, criteria_by_name) | ||
| rescue CsvInvalidLineError |
There was a problem hiding this comment.
I'm not sure what the purpose of this exception handling and re-raising is
| next | ||
| elsif criterion_columns.nil? | ||
| begin | ||
| criterion_columns = assignment_upload_criterion_columns(headers, row, criteria_by_name) |
There was a problem hiding this comment.
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.
| criteria.exists? ? criteria.last.position + 1 : 1 | ||
| end | ||
|
|
||
| def criteria_by_upload_name |
There was a problem hiding this comment.
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.
| 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}' |
There was a problem hiding this comment.
Reword this message to "The following criteria were not found for this assignment: %{criteria}"
3 participants
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)
Add screenshots of:
Associated documentation repository pull request (if applicable)
N/A
Type of Change
Checklist
Before opening your pull request:
After opening your pull request:
Questions and Comments