add support for course_id on groups
fixes CORE-898 test plan - sis import for groups into courses should work Change-Id: I59c9e88c265edd19cd280645cffd27d9b184ca54 Reviewed-on: https://gerrit.instructure.com/138966 Tested-by: Jenkins Reviewed-by: Cody Cutrer <cody@instructure.com> QA-Review: Tucker McKnight <tmcknight@instructure.com> Product-Review: Rob Orton <rob@instructure.com>
This commit is contained in:
Rob Orton
parent
beec413391
commit
3820096c95
|
|
@ -810,7 +810,8 @@ This identifier must not change for the group, and must be globally unique.</td>
|
|||
<td></td>
|
||||
<td></td>
|
||||
<td>The group category identifier from group_categories.csv, if none is
|
||||
specified the group will be put in the default group category for the account.
|
||||
specified the group will be put in the default group category for the account
|
||||
or course or root_account if there is no course_id or account_id.
|
||||
</td>
|
||||
</tr>
|
||||
<tr>
|
||||
|
|
@ -822,6 +823,14 @@ specified the group will be put in the default group category for the account.
|
|||
the root account.</td>
|
||||
</tr>
|
||||
<tr>
|
||||
<td>course_id</td>
|
||||
<td>text</td>
|
||||
<td></td>
|
||||
<td></td>
|
||||
<td>The course identifier from courses.csv, if none is specified the group will be attached to
|
||||
the root account.</td>
|
||||
</tr>
|
||||
<tr>
|
||||
<td>name</td>
|
||||
<td>text</td>
|
||||
<td>✓</td>
|
||||
|
|
|
|||
|
|
@ -36,9 +36,10 @@ module SIS
|
|||
update_progress
|
||||
|
||||
begin
|
||||
importer.add_group(row['group_id'], row['group_category_id'], row['account_id'], row['name'], row['status'])
|
||||
importer.add_group(row['group_id'], row['group_category_id'], row['account_id'],
|
||||
row['course_id'], row['name'], row['status'])
|
||||
rescue ImportError => e
|
||||
add_warning(csv, "#{e}")
|
||||
add_warning(csv, e.to_s)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
|||
|
|
@ -41,24 +41,35 @@ module SIS
|
|||
@accounts_cache = {}
|
||||
end
|
||||
|
||||
def add_group(group_id, group_category_id, account_id, name, status)
|
||||
raise ImportError, "No group_id given for a group" unless group_id.present?
|
||||
def add_group(group_id, group_category_id, account_id, course_id, name, status)
|
||||
raise ImportError, "No group_id given for a group." unless group_id
|
||||
if course_id && account_id
|
||||
raise ImportError, "Only one context is allowed and both course_id and account_id where provided for group #{group_id}."
|
||||
end
|
||||
|
||||
@logger.debug("Processing Group #{[group_id, account_id, name, status].inspect}")
|
||||
@logger.debug("Processing Group #{[group_id, group_category_id, account_id, course_id, name, status].inspect}")
|
||||
|
||||
account = nil
|
||||
if account_id.present?
|
||||
account = @accounts_cache[account_id]
|
||||
account ||= @root_account.all_accounts.where(sis_source_id: account_id).take
|
||||
raise ImportError, "Parent account didn't exist for #{account_id}" unless account
|
||||
@accounts_cache[account.sis_source_id] = account
|
||||
context = nil
|
||||
if account_id
|
||||
context = @accounts_cache[account_id]
|
||||
context ||= @root_account.all_accounts.active.where(sis_source_id: account_id).take
|
||||
raise ImportError, "Account with sis id #{account_id} didn't exist for group #{group_id}." unless context
|
||||
@accounts_cache[context.sis_source_id] = context
|
||||
end
|
||||
|
||||
if course_id
|
||||
context = @root_account.all_courses.active.where(sis_source_id: course_id).take
|
||||
raise ImportError, "Course with sis id #{course_id} didn't exist for group #{group_id}." unless context
|
||||
end
|
||||
|
||||
# if the account_id is present and didn't error then look for group_category in account
|
||||
if account && group_category_id.present?
|
||||
group_category = account.group_categories.where(sis_source_id: group_category_id).take
|
||||
if account_id && group_category_id
|
||||
group_category = context.group_categories.where(sis_source_id: group_category_id).take
|
||||
raise ImportError, "Group Category #{group_category_id} didn't exist in account #{account_id} for group #{group_id}." unless group_category
|
||||
# look for group_category, account doesn't exist
|
||||
elsif course_id && group_category_id
|
||||
group_category = context.group_categories.where(sis_source_id: group_category_id).take
|
||||
raise ImportError, "Group Category #{group_category_id} didn't exist in course #{course_id} for group #{group_id}." unless group_category
|
||||
# look for group_category, account and course don't exist
|
||||
elsif group_category_id.present?
|
||||
group_category = @root_account.all_group_categories.where(deleted_at: nil, sis_source_id: group_category_id).take
|
||||
raise ImportError, "Group Category #{group_category_id} didn't exist for group #{group_id}." unless group_category
|
||||
|
|
@ -66,26 +77,23 @@ module SIS
|
|||
|
||||
group = @root_account.all_groups.where(sis_source_id: group_id).take
|
||||
unless group
|
||||
raise ImportError, "No name given for group #{group_id}, skipping" if name.blank?
|
||||
raise ImportError, "Improper status \"#{status}\" for group #{group_id}, skipping" unless status =~ /\A(available|closed|completed|deleted)/i
|
||||
raise ImportError, "No name given for group #{group_id}." if name.blank?
|
||||
raise ImportError, "Improper status \"#{status}\" for group #{group_id}." unless status =~ /\A(available|closed|completed|deleted)/i
|
||||
end
|
||||
|
||||
# if the group_category exists it is in an account that matches the
|
||||
# groups account_id or is blank, but it should be consistent with the
|
||||
# group_category's account so set the account
|
||||
# if the group_category exists it is in the correct context or the
|
||||
# context is blank, but it should be consistent with the
|
||||
# group_category's context, so assign context
|
||||
if group_category
|
||||
account = group_category.context
|
||||
context = group_category.context
|
||||
group ? group.group_category = group_category : group = group_category.groups.new
|
||||
end
|
||||
# no account_id and no group_category in an account, set to root_account
|
||||
account ||= @root_account
|
||||
group ||= account.groups.new
|
||||
# only update the name on new records, and ones that haven't had their name changed since the last sis import
|
||||
group.name = name if name.present? && (group.new_record? || (!group.stuck_sis_fields.include?(:name)))
|
||||
|
||||
# must set .context, not just .account, since these are account-level groups
|
||||
group.context = account
|
||||
group.sis_source_id = group_id
|
||||
# no account_id, course_id, or group_category, assign context to root_account
|
||||
context ||= @root_account
|
||||
group ||= context.groups.new(name: name, sis_source_id: group_id)
|
||||
# only update the name on groups that haven't had their name changed since the last sis import
|
||||
group.name = name if name.present? && !group.stuck_sis_fields.include?(:name)
|
||||
group.context = context
|
||||
group.sis_batch_id = @batch.id if @batch
|
||||
|
||||
# closed and completed are no longer valid states. Leaving these for
|
||||
|
|
|
|||
|
|
@ -20,7 +20,7 @@ require File.expand_path(File.dirname(__FILE__) + '/../../../spec_helper.rb')
|
|||
|
||||
describe SIS::CSV::GroupImporter do
|
||||
|
||||
before { account_model }
|
||||
before {account_model}
|
||||
|
||||
it "should skip bad content" do
|
||||
process_csv_data_cleanly(
|
||||
|
|
@ -29,35 +29,38 @@ describe SIS::CSV::GroupImporter do
|
|||
)
|
||||
before_count = Group.count
|
||||
importer = process_csv_data(
|
||||
"group_id,account_id,name,status,group_category_id",
|
||||
"group_id,account_id,name,status,group_category_id,course_id",
|
||||
"G001,A001,Group 1,available,",
|
||||
"G001,,Group 1,available,,invalid",
|
||||
"G001,invalid,Group 1,available,,invalid",
|
||||
"G002,A001,Group 1,blerged,",
|
||||
"G003,A001,,available,",
|
||||
"G004,A004,Group 4,available,",
|
||||
",A001,G1,available,",
|
||||
"G006,A001,Group 6,available,invalid",
|
||||
)
|
||||
err = ["Course with sis id invalid didn't exist for group G001.",
|
||||
"Only one context is allowed and both course_id and account_id where provided for group G001.",
|
||||
"Improper status \"blerged\" for group G002.",
|
||||
"No name given for group G003.",
|
||||
"Account with sis id A004 didn't exist for group G004.",
|
||||
"No group_id given for a group.",
|
||||
"Group Category invalid didn't exist in account A001 for group G006."]
|
||||
expect(importer.errors).to eq []
|
||||
expect(importer.warnings.map(&:last)).to eq(
|
||||
["Improper status \"blerged\" for group G002, skipping",
|
||||
"No name given for group G003, skipping",
|
||||
"Parent account didn't exist for A004",
|
||||
"No group_id given for a group",
|
||||
"Group Category invalid didn't exist in account A001 for group G006."]
|
||||
)
|
||||
expect(importer.warnings.map(&:last)).to eq(err)
|
||||
expect(Group.count).to eq before_count + 1
|
||||
end
|
||||
|
||||
it "should create groups" do
|
||||
account_model
|
||||
@sub = @account.all_accounts.create!(:name => 'sub')
|
||||
@sub.update_attribute('sis_source_id', 'A002')
|
||||
sub = @account.all_accounts.create!(name: 'sub')
|
||||
sub.update_attribute('sis_source_id', 'A002')
|
||||
process_csv_data_cleanly(
|
||||
"group_id,account_id,name,status",
|
||||
"G001,,Group 1,available",
|
||||
"G002,A002,Group 2,deleted")
|
||||
groups = Group.order(:id).to_a
|
||||
expect(groups.map(&:account_id)).to eq [@account.id, @sub.id]
|
||||
expect(groups.map(&:account_id)).to eq [@account.id, sub.id]
|
||||
expect(groups.map(&:sis_source_id)).to eq %w(G001 G002)
|
||||
expect(groups.map(&:name)).to eq ["Group 1", "Group 2"]
|
||||
expect(groups.map(&:workflow_state)).to eq %w(available deleted)
|
||||
|
|
@ -76,8 +79,8 @@ describe SIS::CSV::GroupImporter do
|
|||
end
|
||||
|
||||
it "should update group attributes" do
|
||||
@sub = @account.sub_accounts.create!(:name => 'sub')
|
||||
@sub.update_attribute('sis_source_id', 'A002')
|
||||
sub = @account.sub_accounts.create!(name: 'sub')
|
||||
sub.update_attribute('sis_source_id', 'A002')
|
||||
process_csv_data_cleanly(
|
||||
"group_id,account_id,name,status",
|
||||
"G001,,Group 1,available",
|
||||
|
|
@ -93,12 +96,12 @@ describe SIS::CSV::GroupImporter do
|
|||
expect(groups.map(&:name)).to eq ["Group 1-1", "Group 2-b"]
|
||||
expect(groups.map(&:root_account)).to eq [@account, @account]
|
||||
expect(groups.map(&:workflow_state)).to eq %w(available deleted)
|
||||
expect(groups.map(&:account)).to eq [@account, @sub]
|
||||
expect(groups.map(&:account)).to eq [@account, sub]
|
||||
end
|
||||
|
||||
it "should use group_category_id" do
|
||||
@sub = @account.sub_accounts.create!(:name => 'sub')
|
||||
@sub.update_attribute('sis_source_id', 'A002')
|
||||
sub = @account.sub_accounts.create!(name: 'sub')
|
||||
sub.update_attribute('sis_source_id', 'A002')
|
||||
process_csv_data_cleanly(
|
||||
"group_category_id,account_id,category_name,status",
|
||||
"Gc001,,Group Cat 1,active",
|
||||
|
|
@ -108,7 +111,15 @@ describe SIS::CSV::GroupImporter do
|
|||
"G001,,Group 1,available,Gc001",
|
||||
"G002,,Group 2,available,Gc002")
|
||||
groups = Group.order(:id).to_a
|
||||
expect(groups.map(&:account)).to eq [@account, @sub]
|
||||
expect(groups.map(&:account)).to eq [@account, sub]
|
||||
expect(groups.map(&:group_category)).to eq GroupCategory.order(:id).to_a
|
||||
end
|
||||
|
||||
it "should use course_id" do
|
||||
course = course_factory(account: @account, sis_source_id: 'c001')
|
||||
process_csv_data_cleanly(
|
||||
"group_id,course_id,name,status",
|
||||
"G001,c001,Group 1,available")
|
||||
expect(Group.where(sis_source_id: 'G001').take.context).to eq course
|
||||
end
|
||||
end
|
||||
|
|
|
|||
Loading…
Reference in New Issue