[ci no-cached-dist] Handle custom columns properly when importing
When importing a gradebook CSV and and analyzing what changes have been
made, map changes to custom columns properly and do not attempt to
include deleted or hidden columns.
fixes GRADE-1616
Test plan:
- In a course with some students, set up four custom columns
(referred to as C1 through C4); for now, don't change any
settings on them
- In new Gradebook, set some values for the columns
- Adjust them as follows:
- C1: deleted
- C2: hidden
- C3: read-only
- C4: [leave as is]
- Export a CSV
- The CSV should contain C3 and C4 but not the other two
columns (this behavior has not changed)
- Twiddle some assignment scores and custom-column values in the CSV
- Import the edited CSV and check the following on the proposed
changes screen:
- Changes to C4 are shown as expected
- Any changes to assignment scores are shown as expected
- Changes to C3 are *not* shown, and the warning message about
preventing changing read-only columns is shown at the bottom
- Otherwise, everything is as you'd expect: the importer isn't
attempting to replace a custom column with the section name
or perpetrate any similar skulduggery
Change-Id: Ib1ebcec6820120288e67c1652cb5496a8b8240a7
Reviewed-on: https://gerrit.instructure.com/167149
Reviewed-by: Keith Garner <kgarner@instructure.com>
Reviewed-by: Gary Mei <gmei@instructure.com>
Reviewed-by: Derek Bender <djbender@instructure.com>
QA-Review: Gary Mei <gmei@instructure.com>
Product-Review: Keith Garner <kgarner@instructure.com>
Tested-by: Jenkins
This commit is contained in:
Adrian Packel
parent
0536a4b4b5
commit
b71575d6bd
|
|
@ -113,7 +113,7 @@ class GradebookImporter
|
|||
prevented_grading_ungradeable_submission: false,
|
||||
prevented_changing_read_only_column: false
|
||||
}
|
||||
@custom_column_titles_and_indexes = []
|
||||
@parsed_custom_column_data = {}
|
||||
@gradebook_importer_assignments = {}
|
||||
@gradebook_importer_custom_columns = {}
|
||||
|
||||
|
|
@ -194,8 +194,8 @@ class GradebookImporter
|
|||
)
|
||||
end
|
||||
end
|
||||
@gradebook_importer_custom_columns[student.id].each do |student_custom_column_cell|
|
||||
custom_column = custom_gradebook_columns.detect {|custom_col| custom_col.id == student_custom_column_cell['column_id']}
|
||||
@gradebook_importer_custom_columns[student.id].each do |column_id, student_custom_column_cell|
|
||||
custom_column = custom_gradebook_columns.detect {|custom_col| custom_col.id == column_id}
|
||||
datum = custom_column.custom_gradebook_column_data.detect {|custom_column_datum| custom_column_datum.user_id == student.id}
|
||||
student_custom_column_cell['current_content'] = datum&.content
|
||||
end
|
||||
|
|
@ -225,18 +225,17 @@ class GradebookImporter
|
|||
end
|
||||
|
||||
custom_column_ids_to_skip_on_import = []
|
||||
custom_gradebook_columns.each_with_index do |custom_column, index|
|
||||
exclude_custom_column_on_import = @students.all? do |student|
|
||||
custom_column_datum = @gradebook_importer_custom_columns[student.id][index]
|
||||
no_change = (custom_column_datum['current_content'] == custom_column_datum['new_content']) ||
|
||||
(custom_column_datum['current_content'].blank? && custom_column_datum['new_content'].blank?)
|
||||
custom_gradebook_columns.each do |custom_column|
|
||||
no_change = true
|
||||
if @parsed_custom_column_data.include?(custom_column.id) && !custom_column.deleted?
|
||||
no_change = no_change_to_column_values?(column_id: custom_column.id)
|
||||
|
||||
@warning_messages[:prevented_changing_read_only_column] = true if !no_change && custom_column.read_only
|
||||
|
||||
no_change
|
||||
end
|
||||
|
||||
custom_column_ids_to_skip_on_import << custom_column.id if exclude_custom_column_on_import || custom_column.read_only
|
||||
if no_change || exclude_custom_column_on_import(column: custom_column)
|
||||
custom_column_ids_to_skip_on_import << custom_column.id
|
||||
end
|
||||
end
|
||||
|
||||
indexes_to_delete.reverse_each do |index|
|
||||
|
|
@ -248,8 +247,8 @@ class GradebookImporter
|
|||
|
||||
@custom_gradebook_columns = @custom_gradebook_columns.reject { |custom_col| custom_column_ids_to_skip_on_import.include?(custom_col.id) }
|
||||
@students.each do |student|
|
||||
@gradebook_importer_custom_columns[student.id] = @gradebook_importer_custom_columns[student.id].reject do |custom_col|
|
||||
custom_column_ids_to_skip_on_import.include?(custom_col.fetch('column_id'))
|
||||
@gradebook_importer_custom_columns[student.id].reject! do |column_id, _custom_col|
|
||||
custom_column_ids_to_skip_on_import.include?(column_id)
|
||||
end
|
||||
end
|
||||
|
||||
|
|
@ -300,18 +299,21 @@ class GradebookImporter
|
|||
end
|
||||
end
|
||||
|
||||
def process_custom_columns_headers(row)
|
||||
custom_columns = custom_gradebook_columns
|
||||
def process_custom_column_headers(row)
|
||||
row.each_with_index do |header_column, index|
|
||||
cc = custom_columns.detect { |column| column.title == header_column }
|
||||
if cc.present?
|
||||
@custom_column_titles_and_indexes << {title: header_column, index: index, read_only: cc.read_only}
|
||||
end
|
||||
gradebook_column = custom_gradebook_columns.detect { |column| column.title == header_column }
|
||||
next if gradebook_column.blank?
|
||||
|
||||
@parsed_custom_column_data[gradebook_column.id] = {
|
||||
title: header_column,
|
||||
index: index,
|
||||
read_only: gradebook_column.read_only
|
||||
}
|
||||
end
|
||||
end
|
||||
|
||||
def strip_custom_columns(stripped_row)
|
||||
@custom_column_titles_and_indexes.each do |column|
|
||||
@parsed_custom_column_data.each_value do |column|
|
||||
stripped_row.delete(column[:title])
|
||||
end
|
||||
stripped_row
|
||||
|
|
@ -327,8 +329,8 @@ class GradebookImporter
|
|||
def header?(row)
|
||||
return false unless row_has_student_headers? row
|
||||
|
||||
# this is here instead of in process_header() because @custom_column_titles_and_indexes needs to be updated before update_column_count()
|
||||
process_custom_columns_headers(row)
|
||||
# this is here instead of in process_header() because @parsed_custom_column_data needs to be updated before update_column_count()
|
||||
process_custom_column_headers(row)
|
||||
|
||||
update_column_count row
|
||||
|
||||
|
|
@ -339,7 +341,7 @@ class GradebookImporter
|
|||
|
||||
def last_student_info_column(row)
|
||||
# Custom Columns are between section and assignments
|
||||
row[@student_columns - (1 + @custom_column_titles_and_indexes.length)]
|
||||
row[@student_columns - (1 + @parsed_custom_column_data.length)]
|
||||
end
|
||||
|
||||
def row_has_student_headers?(row)
|
||||
|
|
@ -364,7 +366,7 @@ class GradebookImporter
|
|||
end
|
||||
|
||||
# For Custom Columns
|
||||
@student_columns += @custom_column_titles_and_indexes.length
|
||||
@student_columns += @parsed_custom_column_data.length
|
||||
end
|
||||
|
||||
def strip_non_assignment_columns(stripped_row)
|
||||
|
|
@ -466,16 +468,15 @@ class GradebookImporter
|
|||
end
|
||||
|
||||
# custom columns
|
||||
first_custom_column_index = @student_columns - custom_gradebook_columns.length
|
||||
importer_custom_columns = []
|
||||
importer_custom_columns = {}
|
||||
|
||||
custom_gradebook_columns.each_with_index do |cc, index|
|
||||
col_index = index + first_custom_column_index
|
||||
@parsed_custom_column_data.each do |id, custom_column|
|
||||
column_index = custom_column[:index]
|
||||
new_custom_column_data = {
|
||||
'new_content' => row[col_index],
|
||||
'column_id' => cc.id,
|
||||
'new_content' => row[column_index],
|
||||
'column_id' => id
|
||||
}
|
||||
importer_custom_columns << new_custom_column_data
|
||||
importer_custom_columns[id] = new_custom_column_data
|
||||
end
|
||||
|
||||
@gradebook_importer_custom_columns[student.id] = importer_custom_columns
|
||||
|
|
@ -613,7 +614,7 @@ class GradebookImporter
|
|||
:previous_id => student.previous_id,
|
||||
:id => student.id,
|
||||
:submissions => @gradebook_importer_assignments[student.id],
|
||||
:custom_column_data => @gradebook_importer_custom_columns[student.id]
|
||||
:custom_column_data => @gradebook_importer_custom_columns[student.id]&.values
|
||||
}
|
||||
end
|
||||
|
||||
|
|
@ -650,4 +651,17 @@ class GradebookImporter
|
|||
# to avoid an N+1, check that first.
|
||||
is_admin || submission.grants_right?(@user, :grade)
|
||||
end
|
||||
|
||||
def no_change_to_column_values?(column_id:)
|
||||
@students.all? do |student|
|
||||
custom_column_datum = @gradebook_importer_custom_columns[student.id][column_id]
|
||||
|
||||
return true if custom_column_datum['current_content'].blank? && custom_column_datum['new_content'].blank?
|
||||
custom_column_datum['current_content'] == custom_column_datum['new_content']
|
||||
end
|
||||
end
|
||||
|
||||
def exclude_custom_column_on_import(column:)
|
||||
column.deleted? || column.read_only
|
||||
end
|
||||
end
|
||||
|
|
|
|||
|
|
@ -553,6 +553,12 @@ describe GradebookImporter do
|
|||
end
|
||||
|
||||
context "custom gradebook columns" do
|
||||
let(:uploaded_custom_columns) { @gi.upload.gradebook["custom_columns"] }
|
||||
let(:uploaded_student_custom_column_data) do
|
||||
student_data = @gi.upload.gradebook["students"].first
|
||||
student_data["custom_column_data"]
|
||||
end
|
||||
|
||||
before do
|
||||
@student = User.create!
|
||||
course_with_student(course: @course, user: @student, active_enrollment: true)
|
||||
|
|
@ -589,6 +595,80 @@ describe GradebookImporter do
|
|||
col = @gi.upload.gradebook.fetch('students').first.fetch('custom_column_data').map { |custom_column| custom_column.fetch('new_content') }
|
||||
expect(col).to eq ['test 1', 'test 2']
|
||||
end
|
||||
|
||||
it "does not capture custom columns that are not included in the import" do
|
||||
importer_with_rows(
|
||||
"Student,ID,Section,CustomColumn2,Assignment 1",
|
||||
",#{@student.id},,test 2,10"
|
||||
)
|
||||
expect(uploaded_custom_columns).not_to include(hash_including(title: "CustomColumn1"))
|
||||
end
|
||||
|
||||
it "does not attempt to change the values of custom columns that are not included in the import" do
|
||||
importer_with_rows(
|
||||
"Student,ID,Section,CustomColumn2,Assignment 1",
|
||||
",#{@student.id},,test 2,10"
|
||||
)
|
||||
|
||||
column = @course.custom_gradebook_columns.find_by(title: 'CustomColumn1')
|
||||
expect(uploaded_student_custom_column_data).not_to include(hash_including(column_id: column.id))
|
||||
end
|
||||
|
||||
it "captures new values even if custom columns are in different positions" do
|
||||
importer_with_rows(
|
||||
"Student,ID,Section,CustomColumn2,CustomColumn1,Assignment 1",
|
||||
",#{@student.id},,test 2,test 1,10"
|
||||
)
|
||||
|
||||
column = @course.custom_gradebook_columns.find_by(title: 'CustomColumn2')
|
||||
column_datum = uploaded_student_custom_column_data.detect { |datum| datum['column_id'] == column.id }
|
||||
expect(column_datum["new_content"]).to eq "test 2"
|
||||
end
|
||||
|
||||
context "with a deleted custom column" do
|
||||
before(:each) do
|
||||
@course.custom_gradebook_columns.find_by(title: "CustomColumn1").destroy
|
||||
end
|
||||
|
||||
it "omits deleted custom columns when they are included in the import" do
|
||||
importer_with_rows(
|
||||
"Student,ID,Section,CustomColumn1,CustomColumn2,Assignment 1",
|
||||
",#{@student.id},,test 1,test 2,10"
|
||||
)
|
||||
|
||||
expect(uploaded_custom_columns.pluck(:title)).not_to include("CustomColumn1")
|
||||
end
|
||||
|
||||
it "ignores deleted custom columns when they are not included in the import" do
|
||||
importer_with_rows(
|
||||
"Student,ID,Section,CustomColumn2,Assignment 1",
|
||||
",#{@student.id},,test 2,10"
|
||||
)
|
||||
|
||||
expect(uploaded_custom_columns.pluck(:title)).not_to include("CustomColumn1")
|
||||
end
|
||||
|
||||
it "supplies the expected new values for non-deleted columns" do
|
||||
importer_with_rows(
|
||||
"Student,ID,Section,CustomColumn2,Assignment 1",
|
||||
",#{@student.id},,NewCustomColumnValue,10"
|
||||
)
|
||||
|
||||
expect(uploaded_student_custom_column_data.first["new_content"]).to eq "NewCustomColumnValue"
|
||||
end
|
||||
|
||||
it "supplies the expected current values for non-deleted columns" do
|
||||
active_column = @course.custom_gradebook_columns.find_by(title: "CustomColumn2")
|
||||
active_column.custom_gradebook_column_data.create!(user_id: @student.id, content: "OldCustomColumnValue")
|
||||
|
||||
importer_with_rows(
|
||||
"Student,ID,Section,CustomColumn2,Assignment 1",
|
||||
",#{@student.id},,NewCustomColumnValue,10"
|
||||
)
|
||||
|
||||
expect(uploaded_student_custom_column_data.first["current_content"]).to eq "OldCustomColumnValue"
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context "to_json" do
|
||||
|
|
|
|||
Loading…
Reference in New Issue