catch import errors on user preparation
refs FOO-2771 some of the hook implementations may raise errors and we want that to only fail the single row rather than the whole batch Change-Id: I7d7ad5f730f8254abe1ad18b326685820206d7e4 Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/295166 Reviewed-by: Sean Scally <sean.scally@instructure.com> Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com> QA-Review: Ben Rinaca <brinaca@instructure.com> Product-Review: Ben Rinaca <brinaca@instructure.com>
This commit is contained in:
Ben Rinaca
parent
24eb2672bb
commit
ab179e16a4
|
|
@ -147,70 +147,74 @@ module SIS
|
|||
next
|
||||
end
|
||||
|
||||
if pseudo
|
||||
if login_only
|
||||
message = I18n.t("An existing Canvas user with the SIS ID %{user_id} or login of %{login} already exists, skipping", user_id: user_row.user_id, login: user_row.login_id)
|
||||
@messages << SisBatch.build_error(user_row.csv, message, sis_batch: @batch, row: user_row.lineno, row_info: user_row.row)
|
||||
next
|
||||
end
|
||||
if pseudo.sis_user_id && pseudo.sis_user_id != user_row.user_id
|
||||
if @batch.options[:update_sis_id_if_login_claimed]
|
||||
pseudo.sis_user_id = user_row.user_id
|
||||
else
|
||||
message = I18n.t("An existing Canvas user with the SIS ID %{user_id} has already claimed %{other_user_id}'s user_id requested login information, skipping", user_id: pseudo.sis_user_id, other_user_id: user_row.user_id)
|
||||
begin
|
||||
if pseudo
|
||||
if login_only
|
||||
message = I18n.t("An existing Canvas user with the SIS ID %{user_id} or login of %{login} already exists, skipping", user_id: user_row.user_id, login: user_row.login_id)
|
||||
@messages << SisBatch.build_error(user_row.csv, message, sis_batch: @batch, row: user_row.lineno, row_info: user_row.row)
|
||||
next
|
||||
end
|
||||
end
|
||||
if pseudo_by_login && ((pseudo != pseudo_by_login && status != "deleted") ||
|
||||
!Pseudonym.where("LOWER(?)=LOWER(?)", pseudo.unique_id, user_row.login_id).exists?)
|
||||
id_message = pseudo_by_login.sis_user_id ? "SIS ID" : "Canvas ID"
|
||||
user_id = pseudo_by_login.sis_user_id || pseudo_by_login.user_id
|
||||
message = I18n.t("An existing Canvas user with the %{user_id} has already claimed %{other_user_id}'s user_id requested login information, skipping", user_id: "#{id_message} #{user_id}", other_user_id: user_row.user_id)
|
||||
@messages << SisBatch.build_error(user_row.csv, message, sis_batch: @batch, row: user_row.lineno, row_info: user_row.row)
|
||||
next
|
||||
end
|
||||
if pseudo.sis_user_id && pseudo.sis_user_id != user_row.user_id
|
||||
if @batch.options[:update_sis_id_if_login_claimed]
|
||||
pseudo.sis_user_id = user_row.user_id
|
||||
else
|
||||
message = I18n.t("An existing Canvas user with the SIS ID %{user_id} has already claimed %{other_user_id}'s user_id requested login information, skipping", user_id: pseudo.sis_user_id, other_user_id: user_row.user_id)
|
||||
@messages << SisBatch.build_error(user_row.csv, message, sis_batch: @batch, row: user_row.lineno, row_info: user_row.row)
|
||||
next
|
||||
end
|
||||
end
|
||||
if pseudo_by_login && ((pseudo != pseudo_by_login && status != "deleted") ||
|
||||
!Pseudonym.where("LOWER(?)=LOWER(?)", pseudo.unique_id, user_row.login_id).exists?)
|
||||
id_message = pseudo_by_login.sis_user_id ? "SIS ID" : "Canvas ID"
|
||||
user_id = pseudo_by_login.sis_user_id || pseudo_by_login.user_id
|
||||
message = I18n.t("An existing Canvas user with the %{user_id} has already claimed %{other_user_id}'s user_id requested login information, skipping", user_id: "#{id_message} #{user_id}", other_user_id: user_row.user_id)
|
||||
@messages << SisBatch.build_error(user_row.csv, message, sis_batch: @batch, row: user_row.lineno, row_info: user_row.row)
|
||||
next
|
||||
end
|
||||
user = if force_new_user?(user_row, pseudo)
|
||||
new_user(user_row)
|
||||
else
|
||||
pseudo.user
|
||||
end
|
||||
|
||||
user = if force_new_user?(user_row, pseudo)
|
||||
new_user(user_row)
|
||||
else
|
||||
pseudo.user
|
||||
end
|
||||
|
||||
unless user.stuck_sis_fields.include?(:name)
|
||||
user.name = infer_user_name(user_row, user.name)
|
||||
end
|
||||
unless user.stuck_sis_fields.include?(:sortable_name)
|
||||
user.sortable_name = infer_sortable_name(user_row, user.sortable_name)
|
||||
end
|
||||
if !user.stuck_sis_fields.include?(:short_name) && user_row.short_name.present?
|
||||
user.short_name = user_row.short_name
|
||||
end
|
||||
elsif login_only
|
||||
if user_row.root_account_id.present?
|
||||
root_account = root_account_from_id(user_row.root_account_id, user_row)
|
||||
next unless root_account
|
||||
unless user.stuck_sis_fields.include?(:name)
|
||||
user.name = infer_user_name(user_row, user.name)
|
||||
end
|
||||
unless user.stuck_sis_fields.include?(:sortable_name)
|
||||
user.sortable_name = infer_sortable_name(user_row, user.sortable_name)
|
||||
end
|
||||
if !user.stuck_sis_fields.include?(:short_name) && user_row.short_name.present?
|
||||
user.short_name = user_row.short_name
|
||||
end
|
||||
elsif login_only
|
||||
if user_row.root_account_id.present?
|
||||
root_account = root_account_from_id(user_row.root_account_id, user_row)
|
||||
next unless root_account
|
||||
else
|
||||
root_account = @root_account
|
||||
end
|
||||
pseudo = existing_login(user_row, root_account)
|
||||
if pseudo.nil?
|
||||
message = I18n.t("Could not find the existing user for login with SIS ID %{user_id}, skipping", user_id: user_row.user_id)
|
||||
@messages << SisBatch.build_error(user_row.csv, message, sis_batch: @batch, row: user_row.lineno, row_info: user_row.row)
|
||||
next
|
||||
elsif pseudo.attributes.slice(*user_row.login_hash.keys) != user_row.login_hash
|
||||
message = I18n.t("An existing user does not match existing user ids provided for login with SIS ID %{user_id}, skipping", user_id: user_row.user_id)
|
||||
@messages << SisBatch.build_error(user_row.csv, message, sis_batch: @batch, row: user_row.lineno, row_info: user_row.row)
|
||||
next
|
||||
else
|
||||
user = pseudo.user
|
||||
pseudo = Pseudonym.new
|
||||
end
|
||||
else
|
||||
root_account = @root_account
|
||||
end
|
||||
pseudo = existing_login(user_row, root_account)
|
||||
if pseudo.nil?
|
||||
message = I18n.t("Could not find the existing user for login with SIS ID %{user_id}, skipping", user_id: user_row.user_id)
|
||||
@messages << SisBatch.build_error(user_row.csv, message, sis_batch: @batch, row: user_row.lineno, row_info: user_row.row)
|
||||
next
|
||||
elsif pseudo.attributes.slice(*user_row.login_hash.keys) != user_row.login_hash
|
||||
message = I18n.t("An existing user does not match existing user ids provided for login with SIS ID %{user_id}, skipping", user_id: user_row.user_id)
|
||||
@messages << SisBatch.build_error(user_row.csv, message, sis_batch: @batch, row: user_row.lineno, row_info: user_row.row)
|
||||
next
|
||||
else
|
||||
user = pseudo.user
|
||||
user = nil
|
||||
pseudo = Pseudonym.new
|
||||
user = other_user(user_row, pseudo) if user_row.integration_id.present?
|
||||
user = new_user(user_row) if user.blank? || force_new_user?(user_row, pseudo)
|
||||
end
|
||||
else
|
||||
user = nil
|
||||
pseudo = Pseudonym.new
|
||||
user = other_user(user_row, pseudo) if user_row.integration_id.present?
|
||||
user = new_user(user_row) if user.blank? || force_new_user?(user_row, pseudo)
|
||||
rescue ImportError => e
|
||||
@messages << SisBatch.build_error(user_row.csv, e.message, sis_batch: @batch, row: user_row.lineno, row_info: user_row.row)
|
||||
next
|
||||
end
|
||||
|
||||
is_new_user_with_password_notification = user.new_record? && user_row.email.present? && user_row.canvas_password_notification.present? && user_row.authentication_provider_id == "canvas"
|
||||
|
|
|
|||
|
|
@ -48,6 +48,66 @@ describe SIS::UserImporter do
|
|||
end
|
||||
end
|
||||
|
||||
describe SIS::UserImporter::Work do
|
||||
let(:account) { account_model }
|
||||
let(:user_sis_id) { "sis_id" }
|
||||
let(:sis_batch) { account.sis_batches.create! }
|
||||
let(:messages) { [] }
|
||||
let(:user_importer) { SIS::UserImporter::Work.new(sis_batch, account, Rails.logger, messages) }
|
||||
let(:sis_user) do
|
||||
SIS::Models::User.new(user_id: user_sis_id, login_id: "123456", status: "active",
|
||||
full_name: "User One", email: "user1@example.com", integration_id: "iid1234")
|
||||
end
|
||||
|
||||
context "when a matching login exists" do
|
||||
let(:existing_login) { pseudonym_model(account: account, sis_user_id: user_sis_id) }
|
||||
|
||||
before do
|
||||
existing_login
|
||||
end
|
||||
|
||||
context "when the force_new_user? hook implementation raises an ImportError" do
|
||||
let(:error_message) { "existing user sisplosion" }
|
||||
|
||||
before do
|
||||
allow(user_importer).to receive(:force_new_user?).and_raise(SIS::ImportError.new(error_message))
|
||||
user_importer.add_user(sis_user)
|
||||
end
|
||||
|
||||
it "records a sis batch warning" do
|
||||
user_importer.process_batch
|
||||
expect(messages).not_to be_empty
|
||||
expect(messages.first.message).to eq(error_message)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context "when no matching login exists" do
|
||||
context "when there is a user found in an implementation of the other_user hook" do
|
||||
let(:other_user) { user_model }
|
||||
|
||||
before do
|
||||
allow(user_importer).to receive(:other_user).and_return(other_user)
|
||||
end
|
||||
|
||||
context "when the force_new_user? hook implementation raises an ImportError" do
|
||||
let(:error_message) { "other user sisplosion" }
|
||||
|
||||
before do
|
||||
allow(user_importer).to receive(:force_new_user?).and_raise(SIS::ImportError.new(error_message))
|
||||
user_importer.add_user(sis_user)
|
||||
end
|
||||
|
||||
it "records a sis batch warning" do
|
||||
user_importer.process_batch
|
||||
expect(messages).not_to be_empty
|
||||
expect(messages.first.message).to eq(error_message)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
it "must raise ImportError when user doesn't have status" do
|
||||
messages = []
|
||||
account_model
|
||||
|
|
|
|||
Loading…
Reference in New Issue