From 3323d45448d03ba8cf7cef0474de3c821b2aa0d2 Mon Sep 17 00:00:00 2001 From: Rob Orton Date: Thu, 1 Oct 2020 04:07:49 -0600 Subject: [PATCH] create group_and_membership_importer test plan - run the following with group category object and file contents - GroupAndMembershipImporter.create_import_with_attachment(category, file) - it should return a progress - it should import groups and group members closes VICE-813 flag=none Change-Id: Iccc7a4f491d7efbe035057a2915f318fd695deb1 Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/249030 Tested-by: Service Cloud Jenkins Reviewed-by: Matthew Lemon QA-Review: Matthew Lemon Product-Review: Rob Orton --- app/models/attachment.rb | 24 ++- app/models/group_and_membership_importer.rb | 156 ++++++++++++++++++ app/models/group_category.rb | 3 +- app/models/outcome_import.rb | 14 +- app/models/sis_batch.rb | 20 +-- ...59_create_group_and_membership_importer.rb | 29 ++++ lib/sis/csv/import_refactored.rb | 3 +- spec/factories/pseudonym_factory.rb | 1 + .../group_and_membership_importer_spec.rb | 147 +++++++++++++++++ 9 files changed, 362 insertions(+), 35 deletions(-) create mode 100644 app/models/group_and_membership_importer.rb create mode 100644 db/migrate/20200930191659_create_group_and_membership_importer.rb create mode 100644 spec/models/group_and_membership_importer_spec.rb diff --git a/app/models/attachment.rb b/app/models/attachment.rb index f58c37f35f9..be0a6159005 100644 --- a/app/models/attachment.rb +++ b/app/models/attachment.rb @@ -67,6 +67,7 @@ class Attachment < ActiveRecord::Base belongs_to :folder belongs_to :user has_one :account_report, inverse_of: :attachment + has_one :group_and_membership_importer, inverse_of: :attachment has_one :media_object has_many :submission_draft_attachments, inverse_of: :attachment has_many :submissions, -> { active } @@ -1405,15 +1406,32 @@ class Attachment < ActiveRecord::Base clauses << wildcard('attachments.content_type', type + '/', :type => :right) end end - condition_sql = clauses.join(' OR ') + clauses.join(' OR ') end - alias_method :destroy_permanently!, :destroy + # this method is used to create attachments from file uploads that are just + # data files. Used in multiple importers in canvas. + def self.create_data_attachment(context, data, display_name=nil) + context.shard.activate do + Attachment.new.tap do |att| + Attachment.skip_3rd_party_submits(true) + att.context = context + att.display_name = display_name if display_name + Attachments::Storage.store_for_attachment(att, data) + att.save! + end + end + ensure + Attachment.skip_3rd_party_submits(false) + end + + alias destroy_permanently! destroy # file_state is like workflow_state, which was already taken # possible values are: available, deleted def destroy return if self.new_record? - self.file_state = 'deleted' #destroy + + self.file_state = 'deleted' # destroy self.deleted_at = Time.now.utc ContentTag.delete_for(self) MediaObject.where(:attachment_id => self.id).update_all(:attachment_id => nil, :updated_at => Time.now.utc) diff --git a/app/models/group_and_membership_importer.rb b/app/models/group_and_membership_importer.rb new file mode 100644 index 00000000000..4920800b040 --- /dev/null +++ b/app/models/group_and_membership_importer.rb @@ -0,0 +1,156 @@ +# +# Copyright (C) 2020 - present Instructure, Inc. +# +# This file is part of Canvas. +# +# Canvas is free software: you can redistribute it and/or modify it under +# the terms of the GNU Affero General Public License as published by the Free +# Software Foundation, version 3 of the License. +# +# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR +# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more +# details. +# +# You should have received a copy of the GNU Affero General Public License along +# with this program. If not, see . +# + +require 'csv' + +class GroupAndMembershipImporter < ActiveRecord::Base + include Canvas::SoftDeletable + + belongs_to :group_category, inverse_of: :group_and_membership_importers + belongs_to :attachment, inverse_of: :group_and_membership_importer + + attr_accessor :progress, :total_lines, :update_every, :seen_groups, :group_members, :seen_user_ids + + def self.create_import_with_attachment(group_category, file_obj) + import = GroupAndMembershipImporter.create!(group_category: group_category) + att = Attachment.create_data_attachment(import, file_obj, "category_import_#{import.global_id}.csv") + import.attachment = att + import.save! + progress = Progress.create!(context: group_category, tag: "course_group_import", completion: 0.0) + progress.process_job(import, :import_groups_from_attachment, + { strand: ["import_groups_from_attachment", group_category.context.global_id] }) + progress + end + + def import_groups_from_attachment(progress) + @progress = progress + progress.start + csv = begin + file = attachment.open + { fullpath: file.path, :file => attachment.display_name, attachment: attachment } + end + validate_file(csv) + return unless progress.reload.running? + + begin + csv_contents = CSV.read(csv[:fullpath], SIS::CSV::CSVBaseImporter::PARSE_ARGS) + rescue CSV::MalformedCSVError + fail_import(I18n.t("Malformed CSV")) + end + @total_lines = csv_contents.length + @update_every ||= [total_lines / 99.to_f.round(0), 50].max + @seen_groups = {} + @seen_user_ids = Set.new + @group_members = {} + create_groups_and_members(csv_contents) + progress.complete + progress.save! + self.workflow_state = 'completed' + self.save! + end + + def validate_file(csv) + fail_import(I18n.t("Unable to read file")) unless File.file?(csv[:fullpath]) + fail_import(I18n.t("Only CSV files are supported.")) unless File.extname(csv[:fullpath]).casecmp('.csv').zero? + fail_import(I18n.t("Invalid UTF-8")) unless Attachment.valid_utf8?(File.open(csv[:fullpath])) + end + + def fail_import(error) + self.worklfow_state = 'failed' + self.save! + progress.message = error + progress.save! + progress.fail + end + + def create_groups_and_members(rows) + rows.each_with_index do |row, index| + group = group_from_row(row) + next unless group + + user = user_from_row(row) + next unless user + + seen_user_ids.include?(user.id) ? next : validate_user(user) + + seen_user_ids << user.id + group_members[group] ||= [] + group_members[group] << user + persist_memberships if index % 1_000 == 0 && index != 0 + update_progress(index) + end + persist_memberships + end + + def validate_user(user) + # if they have any memberships, we are moving them via delete and add + GroupMembership.where(group_id: group_category.groups.select(:id), user_id: user.id).take&.destroy + end + + def user_from_row(row) + user_id = row['canvas_user_id'] + user_sis_id = row['user_id'] + login_id = row['login_id'] + user = nil + user_scope = User.where(id: group_category.context.participating_students_by_date.where.not(enrollments: { type: 'StudentViewEnrollment' })) + user = user_scope.where(id: user_id).take if user_id + pseudonym_scope = Pseudonym.active.where(account_id: group_category.root_account_id) + user ||= user_scope.where(id: pseudonym_scope.where(sis_user_id: user_sis_id).limit(1).select(:user_id)).take if user_sis_id + user ||= user_scope.where(id: pseudonym_scope.by_unique_id(login_id).limit(1).select(:user_id)).take if login_id + user + end + + def group_from_row(row) + group_id = row['canvas_group_id'] + group_sis_id = row['group_id'] + group_name = row['group_name'] + key = group_key(group_id, group_sis_id, group_name) + return unless key + + group = seen_groups[key] + group ||= group_category.groups.where(id: group_id).take if group_id + group ||= group_category.groups.where(sis_source_id: group_sis_id).take if group_sis_id + if group_name + group ||= group_category.groups.where(name: group_name).take + group ||= group_category.groups.create!(name: group_name, context: group_category.context) + end + seen_groups[key] ||= group + group + end + + def group_key(group_id, group_sis_id, group_name) + key = [] + key << "id:#{group_id}" if group_id + key << "sis_id:#{group_sis_id}" if group_sis_id + key << "name:#{group_name}" if group_name + key.join(",").presence + end + + def persist_memberships + group_members.each do |group, users| + group.bulk_add_users_to_group(users) + end + @group_members = {} + end + + def update_progress(index) + if index % update_every == 0 + progress.calculate_completion!(index, total_lines) + end + end +end diff --git a/app/models/group_category.rb b/app/models/group_category.rb index 36f1a9c82f6..7e666dc9ac8 100644 --- a/app/models/group_category.rb +++ b/app/models/group_category.rb @@ -26,10 +26,11 @@ class GroupCategory < ActiveRecord::Base belongs_to :root_account, class_name: 'Account', inverse_of: :all_group_categories has_many :groups, :dependent => :destroy has_many :progresses, :as => 'context', :dependent => :destroy + has_many :group_and_membership_importers, dependent: :destroy, inverse_of: :group_category has_one :current_progress, -> { where(workflow_state: ['queued', 'running']).order(:created_at) }, as: :context, inverse_of: :context, class_name: 'Progress' before_validation :set_root_account_id - validates_uniqueness_of :sis_source_id, scope: [:root_account_id], conditions: -> { where.not(sis_source_id: nil) } + validates :sis_source_id, uniqueness: {scope: :root_account}, allow_nil: true after_save :auto_create_groups after_update :update_groups_max_membership diff --git a/app/models/outcome_import.rb b/app/models/outcome_import.rb index ded1842a02c..d3c0e151d5d 100644 --- a/app/models/outcome_import.rb +++ b/app/models/outcome_import.rb @@ -58,7 +58,7 @@ class OutcomeImport < ApplicationRecord user: user ) - att = create_data_attachment(import, attachment, "outcome_upload_#{import.global_id}.csv") + att = Attachment.create_data_attachment(import, attachment, "outcome_upload_#{import.global_id}.csv") import.attachment = att yield import if block_given? @@ -68,18 +68,6 @@ class OutcomeImport < ApplicationRecord import end - def self.create_data_attachment(import, data, display_name) - Attachment.new.tap do |att| - Attachment.skip_3rd_party_submits(true) - att.context = import - att.display_name = display_name - Attachments::Storage.store_for_attachment(att, data) - att.save! - end - ensure - Attachment.skip_3rd_party_submits(false) - end - def as_json(_options={}) data = { "id" => self.id, diff --git a/app/models/sis_batch.rb b/app/models/sis_batch.rb index aae37b49a0d..4f7d8234d78 100644 --- a/app/models/sis_batch.rb +++ b/app/models/sis_batch.rb @@ -73,7 +73,7 @@ class SisBatch < ActiveRecord::Base batch.user = user batch.save - att = create_data_attachment(batch, attachment) + att = Attachment.create_data_attachment(batch, attachment) batch.attachment = att yield batch if block_given? @@ -84,20 +84,6 @@ class SisBatch < ActiveRecord::Base end end - def self.create_data_attachment(batch, data, display_name=nil) - batch.shard.activate do - Attachment.new.tap do |att| - Attachment.skip_3rd_party_submits(true) - att.context = batch - att.display_name = display_name if display_name - Attachments::Storage.store_for_attachment(att, data) - att.save! - end - end - ensure - Attachment.skip_3rd_party_submits(false) - end - def self.add_error(csv, message, sis_batch:, row: nil, failure: false, backtrace: nil, row_info: nil) error = build_error(csv, message, row: row, failure: failure, backtrace: backtrace, row_info: row_info, sis_batch: sis_batch) error.save! @@ -356,7 +342,7 @@ class SisBatch < ActiveRecord::Base self.data[:diffed_against_sis_batch_id] = previous_batch.id - self.generated_diff = SisBatch.create_data_attachment( + self.generated_diff = Attachment.create_data_attachment( self, Rack::Test::UploadedFile.new(diffed_data_file.path, 'application/zip'), t(:diff_filename, "sis_upload_diffed_%{id}.zip", :id => self.id) @@ -733,7 +719,7 @@ class SisBatch < ActiveRecord::Base csv << row end end - self.errors_attachment = SisBatch.create_data_attachment( + self.errors_attachment = Attachment.create_data_attachment( self, Rack::Test::UploadedFile.new(file, 'csv', true), "sis_errors_attachment_#{id}.csv" diff --git a/db/migrate/20200930191659_create_group_and_membership_importer.rb b/db/migrate/20200930191659_create_group_and_membership_importer.rb new file mode 100644 index 00000000000..7cd9d35075a --- /dev/null +++ b/db/migrate/20200930191659_create_group_and_membership_importer.rb @@ -0,0 +1,29 @@ +# +# Copyright (C) 2020 - present Instructure, Inc. +# +# This file is part of Canvas. +# +# Canvas is free software: you can redistribute it and/or modify it under +# the terms of the GNU Affero General Public License as published by the Free +# Software Foundation, version 3 of the License. +# +# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR +# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more +# details. +# +# You should have received a copy of the GNU Affero General Public License along +# with this program. If not, see . + +class CreateGroupAndMembershipImporter < ActiveRecord::Migration[5.2] + tag :predeploy + + def change + create_table :group_and_membership_importers do |t| + t.references :group_category, foreign_key: true, index: true, null: false, limit: 8 + t.references :attachment, foreign_key: true, index: false, limit: 8 + t.string :workflow_state, null: false, default: 'active' + t.timestamps null: false + end + end +end diff --git a/lib/sis/csv/import_refactored.rb b/lib/sis/csv/import_refactored.rb index 3256d7b0c9f..459612b35b5 100644 --- a/lib/sis/csv/import_refactored.rb +++ b/lib/sis/csv/import_refactored.rb @@ -165,8 +165,9 @@ module SIS def create_batch_attachment(path) return if File.stat(path).size == 0 + data = Rack::Test::UploadedFile.new(path, Attachment.mimetype(path)) - SisBatch.create_data_attachment(@batch, data, File.basename(path)) + Attachment.create_data_attachment(@batch, data, File.basename(path)) end def process diff --git a/spec/factories/pseudonym_factory.rb b/spec/factories/pseudonym_factory.rb index 2062378e816..7f072272b41 100644 --- a/spec/factories/pseudonym_factory.rb +++ b/spec/factories/pseudonym_factory.rb @@ -50,6 +50,7 @@ module Factories password = nil if password == :autogenerate account = (opts[:account] ? opts[:account].root_account : Account.default) @pseudonym = account.pseudonyms.build(:user => user, :unique_id => username, :password => password, :password_confirmation => password) + @pseudonym.sis_user_id = opts[:sis_user_id] @pseudonym.save_without_session_maintenance opts[:username] = opts[:username] + user.id.to_s + '@example.com' unless opts[:username].include? '@' @pseudonym.communication_channel = communication_channel(user, opts) diff --git a/spec/models/group_and_membership_importer_spec.rb b/spec/models/group_and_membership_importer_spec.rb new file mode 100644 index 00000000000..f898cd2a811 --- /dev/null +++ b/spec/models/group_and_membership_importer_spec.rb @@ -0,0 +1,147 @@ +# +# Copyright (C) 2020 - present Instructure, Inc. +# +# This file is part of Canvas. +# +# Canvas is free software: you can redistribute it and/or modify it under +# the terms of the GNU Affero General Public License as published by the Free +# Software Foundation, version 3 of the License. +# +# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR +# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more +# details. +# +# You should have received a copy of the GNU Affero General Public License along +# with this program. If not, see . + +require 'tmpdir' +require 'spec_helper' + +describe GroupAndMembershipImporter do + let_once(:account) { Account.default } + let(:gc1) { @course.group_categories.create!(name: 'gc1') } + let(:group1) { gc1.groups.create!(name: 'manual group', sis_source_id: 'mg1', context: gc1.context) } + + before(:once) do + course_factory(active_course: true) + 5.times do |n| + @course.enroll_user(user_with_pseudonym(sis_user_id: "user_#{n}", username: "login_#{n}"), "StudentEnrollment", enrollment_state: 'active') + end + end + + def create_group_import(data) + Dir.mktmpdir("sis_rspec") do |tmpdir| + path = "#{tmpdir}/csv_0.csv" + File.write(path, data) + + import = File.open(path, 'rb') do |tmp| + # ignore some attachment.rb... stuff + def tmp.original_filename + File.basename(path) + end + + GroupAndMembershipImporter.create_import_with_attachment(gc1, tmp) + end + yield import if block_given? + import + end + end + + def import_csv_data(data) + create_group_import(data) do |progress| + run_jobs + progress.reload + end + end + + context "imports groups" do + it "should return a progress" do + progress = create_group_import(%{user_id,group_name + user_0, first group + user_1, second group + user_2, third group + user_3, third group + user_4, first group}) + expect(progress.class_name).to eq 'Progress' + end + + it 'should work' do + progress = import_csv_data(%{user_id,group_name + user_0, first group + user_1, second group + user_2, third group + user_3, third group + user_4, first group}) + expect(gc1.groups.pluck(:name).sort).to eq ["first group", "second group", "third group"] + expect(Pseudonym.where(user: gc1.groups.where(name: 'first group').take.users).pluck(:sis_user_id).sort).to eq ["user_0", "user_4"] + expect(Pseudonym.where(user: gc1.groups.where(name: 'second group').take.users).pluck(:sis_user_id)).to eq ["user_1"] + expect(Pseudonym.where(user: gc1.groups.where(name: 'third group').take.users).pluck(:sis_user_id).sort).to eq ["user_2", "user_3"] + expect(progress.completion).to eq 100.0 + expect(progress.workflow_state).to eq 'completed' + end + + it 'should skip invalid_users' do + progress = import_csv_data(%{user_id,group_name + user_0, first group + invalid, first group + user_2, first group}) + expect(Pseudonym.where(user: gc1.groups.where(name: 'first group').take.users).pluck(:sis_user_id).sort).to eq ["user_0", "user_2"] + expect(progress.completion).to eq 100.0 + expect(progress.workflow_state).to eq 'completed' + end + + it 'should ignore extra columns' do + progress = import_csv_data(%{user_id,group_name,sections + user_0, first group,sections + user_4, first group,"s1,s2"}) + expect(gc1.groups.count).to eq 1 + expect(Pseudonym.where(user: gc1.groups.where(name: 'first group').take.users).pluck(:sis_user_id).sort).to eq ["user_0", "user_4"] + expect(progress.completion).to eq 100.0 + expect(progress.workflow_state).to eq 'completed' + end + + it 'should ignore invalid groups' do + progress = import_csv_data(%{user_id,group_id + user_0, invalid + user_4,#{group1.sis_source_id}}) + expect(gc1.groups.count).to eq 1 + expect(@user.groups.pluck(:name)).to eq ["manual group"] + expect(progress.completion).to eq 100.0 + expect(progress.workflow_state).to eq 'completed' + end + + it 'should find users by id' do + import_csv_data(%{canvas_user_id,group_name + #{@user.id}, first group}) + expect(@user.groups.pluck(:name)).to eq ["first group"] + end + + it 'should find users by login_id' do + import_csv_data(%{login_id,group_name + #{@user.pseudonym.unique_id}, first group}) + expect(@user.groups.pluck(:name)).to eq ["first group"] + end + + it 'should find existing groups' do + import_csv_data(%{user_id,group_name + user_4,#{group1.name}}) + expect(gc1.groups.count).to eq 1 + expect(@user.groups.pluck(:name)).to eq ["manual group"] + end + + it 'should find existing group by sis_id' do + import_csv_data(%{user_id,group_id + user_4,#{group1.sis_source_id}}) + expect(gc1.groups.count).to eq 1 + expect(@user.groups.pluck(:name)).to eq ["manual group"] + end + + it 'should find existing group by id' do + import_csv_data(%{user_id,canvas_group_id + user_4,#{group1.id}}) + expect(gc1.groups.count).to eq 1 + expect(@user.groups.pluck(:name)).to eq ["manual group"] + end + end +end