feat(gradebook): import header tolerance, column ordering, reassigned-identifier detection, and batched writes#8459
Conversation
…-identifier detection, and batched writes
![hound[bot]](https://avatars.githubusercontent.com/in/3598?s=60&v=4){kind=small}
| it 'overwrites an existing grade when onConflict is replace' do | ||
| post :create, params: base_params.merge(onConflict: 'replace') | ||
| post :create, params: base_params.merge(onConflict: 'replace', | ||
| csvData: "External ID,Midterm\nA001,99\n") |
There was a problem hiding this comment.
[Correctable] Layout/ArgumentAlignment: Align the arguments of a method call if they span more than one line.
[Correctable] Layout/HashAlignment: Align the keys of a hash literal if they span more than one line.
| sql.include?(table) | ||
| end | ||
|
|
||
| ActiveSupport::Notifications.subscribed(counter, 'sql.active_record') { yield } |
There was a problem hiding this comment.
[Correctable] Style/ExplicitBlockArgument: Consider using explicit block argument in the surrounding method's signature over yield.
| expect(error.payload[:suggestions]).to be_empty | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
[Correctable] Layout/TrailingWhitespace: Trailing whitespace detected.
| expect do | ||
| expect do | ||
| service(csv_data: csv, components: components).commit(on_conflict: 'replace') | ||
| end.to raise_error(described_class::ImportError) do |error| |
There was a problem hiding this comment.
Style/MultilineBlockChain: Avoid multi-line chains of blocks.
| component: "Midterms", | ||
| identifier: "S123", | ||
| grade: -2.0, | ||
| kind: "below", |
There was a problem hiding this comment.
[Correctable] Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
| expect(result[:out_of_range]).to include( | ||
| a_hash_including( | ||
| component: "Midterms", | ||
| identifier: "S123", |
There was a problem hiding this comment.
[Correctable] Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
| expect(result[:out_of_range]).to include(a_string_matching(/Midterms: 105.* exceeds 100/)) | ||
| expect(result[:out_of_range]).to include( | ||
| a_hash_including( | ||
| component: "Midterms", |
There was a problem hiding this comment.
[Correctable] Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
| it 'caps the sample at 5 rows but reports the true total in total_rows' do | ||
| extra = (1..5).map { |i| create(:course_student, course: course, external_id: "X00#{i}") } | ||
| ids = ['A001', 'A002'] + extra.map(&:external_id) | ||
| csv = "External ID,Midterm\n" + ids.map { |id| "#{id},10" }.join("\n") + "\n" |
There was a problem hiding this comment.
Style/StringConcatenation: Prefer string interpolation to string concatenation.
| csv = "External ID,Midterm\nA001,41\nA002,37\n" | ||
| expect { service(csv_data: csv, components: components).preview }. | ||
| not_to(change { Course::ExternalAssessmentGrade.count }) | ||
| to not_change { Course::ExternalAssessmentGrade.count }. |
There was a problem hiding this comment.
Lint/AmbiguousBlockAssociation: Parenthesize the param not_change { Course::ExternalAssessment.count } to make sure that the block will be associated with the not_change method call.
| describe '#preview' do | ||
| it 'writes nothing (dry-run)' do | ||
| csv = "External ID,Midterm\nA001,41\nA002,37\n" | ||
| csv = "External ID,Midterm\nA001,41\nA002,37\n" |
There was a problem hiding this comment.
[Correctable] Layout/IndentationWidth: Use 2 (not 1) spaces for indentation.
|
Superseded by #8465 (import correctness + UX consolidated into a single import PR). |
1 participant
Summary
Backend correctness and performance for the external grade importer. Headers are accepted in any order with duplicate-header reporting and near-miss ("did you mean") suggestions; the preview reports columns in the file's own order plus a row count. Conflicts are regrouped into per-student changed-cell rows, and reassigned identifiers (an External ID now pointing at a different student) are detected. Write performance is improved by preloading student emails (N+1 fix) and batching grade writes.
Design decisions
Regression prevention
Covers (Ruby): header order tolerance, duplicate-header reporting, near-miss header suggestion, column ordering from preview, reassigned-identifier detection, email-preload N+1, and batched writes (import service spec + imports controller spec). Backend-only - no frontend change. Specs run on push/CI.