ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

refactor sis imports api 

basically, use the API from the UI, and delete duplicate code.
the api specs already covered everything the non-api specs tested

also drop the unused sis_batch_log_entries table

test plan:
 * validate an sis upload from the UI works, and shows progress,
   and completes successfully
 * validate the ui shows progress when the page is reloaded and
   a batch is in progress

Change-Id: I9be1ac5901a5c6a1b01719718eeda106aaf3045d
Reviewed-on: https://gerrit.instructure.com/11595
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Brian Palmer <brianp@instructure.com>
This commit is contained in:
Cody Cutrer 2012-06-13 16:26:33 -06:00
parent 8ac4a5882e
commit df97e23785
10 changed files with 40 additions and 169 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

42
app/controllers/accounts_controller.rb
View File

@ -287,51 +287,11 @@ class AccountsController < ApplicationController
@terms = @account.enrollment_terms.active
respond_to do |format|
format.html
format.json { render :json => @current_batch.to_json(:include => :sis_batch_log_entries) }
format.json { render :json => @current_batch.try(:api_json) }
end
end
end
def sis_import_submit
raise "SIS imports can only be executed on root accounts" unless @account.root_account?
raise "SIS imports can only be executed on enabled accounts" unless @account.allow_sis_import
if authorized_action(@account, @current_user, :manage_sis)
SisBatch.transaction do
if !@account.current_sis_batch || !@account.current_sis_batch.importing?
batch = SisBatch.create_with_attachment(@account, params[:import_type], params[:attachment])
if params[:batch_mode].to_i > 0
batch.batch_mode = true
if params[:batch_mode_term_id].present?
batch.batch_mode_term = @account.enrollment_terms.active.find(params[:batch_mode_term_id])
end
end
batch.options ||= {}
if params[:override_sis_stickiness].to_i > 0
batch.options[:override_sis_stickiness] = true
[:add_sis_stickiness, :clear_sis_stickiness].each do |option|
batch.options[option] = true if params[option].to_i > 0
end
end
batch.save!
@account.current_sis_batch_id = batch.id
@account.save
batch.process
render :json => batch.to_json(:include => :sis_batch_log_entries),
:as_text => true
else
render :json => {:error=>true, :error_message=> t(:sis_import_in_process_notice, "An SIS import is already in process."), :batch_in_progress=>true}.to_json,
:as_text => true
end
end
end
end
def courses_redirect
redirect_to course_url(params[:id])
end

16
app/controllers/sis_imports_api_controller.rb
View File

@ -91,9 +91,15 @@ class SisImportsApiController < ApplicationController
#
# @argument clear_sis_stickiness ["1"] This option, if present, will clear "stickiness" from all fields touched by this import. Requires that 'override_sis_stickiness' is also provided. If 'add_sis_stickiness' is also provided, 'clear_sis_stickiness' will overrule the behavior of 'add_sis_stickiness'
def create
if authorized_action(@account, @current_user, :manage)
if authorized_action(@account, @current_user, :manage_sis)
params[:import_type] ||= 'instructure_csv'
raise "invalid import type parameter" unless SisBatch.valid_import_types.has_key?(params[:import_type])
if !api_request? && @account.current_sis_batch.try(:importing?)
return render :json => {:error=>true, :error_message=> t(:sis_import_in_process_notice, "An SIS import is already in process."), :batch_in_progress=>true}.to_json,
:as_text => true
end
file_obj = nil
if params.has_key?(:attachment)
file_obj = params[:attachment]
@ -156,6 +162,12 @@ class SisImportsApiController < ApplicationController
unless Setting.get('skip_sis_jobs_account_ids', '').split(',').include?(@account.global_id.to_s)
batch.process
end
unless api_request?
@account.current_sis_batch_id = batch.id
@account.save
end
render :json => batch.api_json
end
end
@ -164,7 +176,7 @@ class SisImportsApiController < ApplicationController
#
# Get the status of an already created SIS import.
def show
if authorized_action(@account, @current_user, :manage)
if authorized_action(@account, @current_user, :manage_sis)
@batch = SisBatch.find(params[:id])
raise "Sis Import not found" unless @batch
raise "Batch does not match account" unless @batch.account.id == @account.id

2
app/models/sis_batch.rb
View File

@ -19,7 +19,6 @@
class SisBatch < ActiveRecord::Base
include Workflow
belongs_to :account
has_many :sis_batch_log_entries, :order => :created_at
serialize :data
serialize :options
serialize :processing_errors, Array
@ -205,7 +204,6 @@ class SisBatch < ActiveRecord::Base
}
data["processing_errors"] = self.processing_errors if self.processing_errors.present?
data["processing_warnings"] = self.processing_warnings if self.processing_warnings.present?
data["sis_batch_log_entries"] = self.sis_batch_log_entries if self.sis_batch_log_entries.present?
return data.to_json
end

33
app/models/sis_batch_log_entry.rb
View File

@ -1,33 +0,0 @@
#
# Copyright (C) 2011 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 <http://www.gnu.org/licenses/>.
#
class SisBatchLogEntry < ActiveRecord::Base
validates_length_of :text, :maximum => maximum_text_length, :allow_nil => true, :allow_blank => true
belongs_to :sis_batch
attr_accessible :text, :sis_batch
def text=(val)
if !val || val.length < self.class.maximum_text_length
write_attribute(:text, val)
else
write_attribute(:text, val[0,self.class.maximum_text_length])
end
end
end

2
app/views/accounts/sis_import.html.erb
View File

@ -15,7 +15,7 @@
</div>
<div class="form" style="<%= hidden if @current_batch && @current_batch.importing? %>">
<% form_tag account_sis_import_submit_path(@account.id), :multipart => true, :id => "sis_importer" do %>
<% form_tag account_sis_imports_path(@account.id), :multipart => true, :id => "sis_importer" do %>
<p class="instruction"><%= mt(:select_file_instructions,
"Select the zip file that you want imported. \n" +
"For a description of how to generate these zip files, [please see this documentation](%{uri}).",

4
config/routes.rb
View File

@ -422,8 +422,8 @@ ActionController::Routing::Routes.draw do |map|
account.resources :terms
account.resources :sub_accounts
account.avatars 'avatars', :controller => 'accounts', :action => 'avatars'
account.sis_import 'sis_import', :controller => 'accounts', :action => 'sis_import'
account.sis_import_submit 'sis_import_submit', :controller => 'accounts', :action => 'sis_import_submit'
account.sis_import 'sis_import', :controller => 'accounts', :action => 'sis_import', :conditions => { :method => :get }
account.resources :sis_imports, :controller => 'sis_imports_api', :only => [:create, :show]
account.add_user 'users', :controller => 'users', :action => 'create', :conditions => {:method => :post}
account.confirm_delete_user 'users/:user_id/delete', :controller => 'accounts', :action => 'confirm_delete_user'
account.delete_user 'users/:user_id', :controller => 'accounts', :action => 'remove_user', :conditions => {:method => :delete}

17
db/migrate/20120613214030_drop_sis_batch_log_entries.rb Normal file
View File

@ -0,0 +1,17 @@
class DropSisBatchLogEntries < ActiveRecord::Migration
tag :postdeploy
def self.up
drop_table :sis_batch_log_entries
end
def self.down
create_table "sis_batch_log_entries", :force => true do |t|
t.integer "sis_batch_id", :limit => 8
t.string "log_type"
t.text "text"
t.datetime "created_at"
t.datetime "updated_at"
end
end
end

21
public/javascripts/sis_import.js
View File

@ -118,27 +118,12 @@ $(document).ready(function(event) {
var waitTime = 1500;
$.ajaxJSON(location.href, 'GET', {}, function(data) {
state = "updating";
var sis_batch = data.sis_batch;
var sis_batch = data;
var progress = 0;
if(sis_batch) {
progress = Math.max($(".copy_progress").progressbar('option', 'value') || 0, sis_batch.progress);
$(".copy_progress").progressbar('option', 'value', progress);
$("#import_log").empty();
if(sis_batch.sis_batch_log_entries) {
for(var idx in sis_batch.sis_batch_log_entries) {
var entry = sis_batch.sis_batch_log_entries[idx].sis_batch_log_entry;
var lines = entry.text.split("\\n");
if($("#import_log #log_" + entry.id).length == 0) {
var $holder = $("<div id='log_" + entry.id + "'/>");
for(var jdx in lines) {
var $div = $("<div/>");
$div.text(lines[jdx]);
$holder.append($div);
}
$("#import_log").append($holder);
}
}
}
}
if(!sis_batch || sis_batch.workflow_state == 'imported') {
$("#sis_importer").hide();
@ -185,7 +170,7 @@ $(document).ready(function(event) {
$("#sis_importer").formSubmit({
fileUpload: true,
success: function(data) {
if(data && data.sis_batch) {
if(data && data.id) {
startPoll();
} else {
//show error message
@ -206,7 +191,7 @@ $(document).ready(function(event) {
state = "checking";
$.ajaxJSON(location.href, 'GET', {}, function(data) {
state = "nothing";
var sis_batch = data.sis_batch;
var sis_batch = data;
var progress = 0;
if(sis_batch && (sis_batch.workflow_state == "importing" || sis_batch.workflow_state == "created")) {
state = "nothing";

68
spec/controllers/accounts_controller_spec.rb
View File

@ -128,74 +128,6 @@ describe AccountsController do
end
end
describe "SIS imports" do
it "should set batch mode and term if given" do
account_with_admin_logged_in
@account.update_attribute(:allow_sis_import, true)
post 'sis_import_submit', :account_id => @account.id, :import_type => 'instructure_csv', :batch_mode => '1'
batch = SisBatch.last
batch.should_not be_nil
batch.batch_mode.should be_true
batch.batch_mode_term.should be_nil
batch.destroy
post 'sis_import_submit', :account_id => @account.id, :import_type => 'instructure_csv', :batch_mode => '1', :batch_mode_term_id => @account.enrollment_terms.first.id
batch = SisBatch.last
batch.should_not be_nil
batch.batch_mode.should be_true
batch.batch_mode_term.should == @account.enrollment_terms.first
end
it "should set sis stickiness options if given" do
account_with_admin_logged_in
@account.update_attribute(:allow_sis_import, true)
post 'sis_import_submit', :account_id => @account.id,
:import_type => 'instructure_csv'
batch = SisBatch.last
batch.should_not be_nil
batch.options.should == {}
batch.destroy
post 'sis_import_submit', :account_id => @account.id,
:import_type => 'instructure_csv', :override_sis_stickiness => '1'
batch = SisBatch.last
batch.should_not be_nil
batch.options.should == { :override_sis_stickiness => true }
batch.destroy
post 'sis_import_submit', :account_id => @account.id,
:import_type => 'instructure_csv', :override_sis_stickiness => '1',
:add_sis_stickiness => '1'
batch = SisBatch.last
batch.should_not be_nil
batch.options.should == { :override_sis_stickiness => true, :add_sis_stickiness => true }
batch.destroy
post 'sis_import_submit', :account_id => @account.id,
:import_type => 'instructure_csv', :override_sis_stickiness => '1',
:clear_sis_stickiness => '1'
batch = SisBatch.last
batch.should_not be_nil
batch.options.should == { :override_sis_stickiness => true, :clear_sis_stickiness => true }
batch.destroy
post 'sis_import_submit', :account_id => @account.id,
:import_type => 'instructure_csv', :clear_sis_stickiness => '1'
batch = SisBatch.last
batch.should_not be_nil
batch.options.should == {}
batch.destroy
post 'sis_import_submit', :account_id => @account.id,
:import_type => 'instructure_csv', :add_sis_stickiness => '1'
batch = SisBatch.last
batch.should_not be_nil
batch.options.should == {}
batch.destroy
end
end
describe "add_account_user" do
it "should allow adding a new account admin" do
account_with_admin_logged_in

4
spec/selenium/sis_imports_spec.rb
View File

@ -120,7 +120,7 @@ describe "sis imports ui" do
submit_form('#sis_importer')
keep_trying_until { driver.find_element(:css, 'div.progress_bar_holder div.progress_message').should be_displayed }
SisBatch.last.process_without_send_later
keep_trying_until { driver.find_element(:css, "div.sis_messages div.error_message").text =~ /There was an error importing your SIS data\./ }
keep_trying_until { driver.find_element(:css, "div.sis_messages div.error_message").text =~ /No SIS records were imported. The import failed with these messages:/ }
SisBatch.last.batch_mode.should == true
SisBatch.last.options.should == {:override_sis_stickiness => true,
:add_sis_stickiness => true}
@ -130,7 +130,7 @@ describe "sis imports ui" do
submit_form('#sis_importer')
keep_trying_until { driver.find_element(:css, 'div.progress_bar_holder div.progress_message').should be_displayed }
SisBatch.last.process_without_send_later
keep_trying_until { driver.find_element(:css, "div.sis_messages div.error_message").text =~ /There was an error importing your SIS data\./ }
keep_trying_until { driver.find_element(:css, "div.sis_messages div.error_message").text =~ /No SIS records were imported. The import failed with these messages:/ }
(!!SisBatch.last.batch_mode).should be_false
SisBatch.last.options.should == {:override_sis_stickiness => true}
end