don't allow multiple root folders for courses
change the way we create the default course root folder
("course files") to use a atomic sql statement
this should prevent suspected race conditions from creating
multiple root folders for a course, as this causes the
files page to become unusable
test plan:
* Unfortunately there seems to be no consistent way to
reproduce the errors, and thus no consistent way to
test that this fixes the error
Change-Id: I223a2b230921752ef1175c82adfdd854e1331668
Reviewed-on: https://gerrit.instructure.com/17014
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Jeremy Stanley <jeremy@instructure.com>
QA-Review: Bracken Mosbacker <bracken@instructure.com>
This commit is contained in:
James Williams
parent
3099b9cca3
commit
67b9f83722
|
|
@ -113,9 +113,10 @@ class Folder < ActiveRecord::Base
|
|||
def default_values
|
||||
self.last_unlock_at = self.unlock_at if self.unlock_at
|
||||
self.last_lock_at = self.lock_at if self.lock_at
|
||||
# You can't lock or hide root folders
|
||||
if !self.parent_folder_id && (self.locked? || self.hidden? || self.protected?)
|
||||
self.workflow_state = 'visible'
|
||||
|
||||
if self.parent_folder_id.blank? && ![ROOT_FOLDER_NAME, MY_FILES_FOLDER_NAME, 'files'].include?(self.name)
|
||||
root_folder = Folder.root_folders(context).first
|
||||
self.parent_folder_id = root_folder.id
|
||||
end
|
||||
end
|
||||
|
||||
|
|
@ -247,25 +248,22 @@ class Folder < ActiveRecord::Base
|
|||
end
|
||||
|
||||
def self.root_folders(context)
|
||||
root_folders = []
|
||||
root_folders = context.folders.active.find_all_by_parent_folder_id(nil)
|
||||
if context.is_a? Course
|
||||
if root_folders.select{|f| f.name == ROOT_FOLDER_NAME }.empty?
|
||||
root_folders << context.folders.create(:name => ROOT_FOLDER_NAME, :full_name => ROOT_FOLDER_NAME, :workflow_state => "visible")
|
||||
end
|
||||
name = ROOT_FOLDER_NAME
|
||||
elsif context.is_a? User
|
||||
# TODO i18n
|
||||
t :my_files_folder_name, 'my files'
|
||||
if root_folders.select{|f| f.name == MY_FILES_FOLDER_NAME }.empty?
|
||||
root_folders << context.folders.create(:name => MY_FILES_FOLDER_NAME, :full_name => MY_FILES_FOLDER_NAME, :workflow_state => "visible")
|
||||
end
|
||||
name = MY_FILES_FOLDER_NAME
|
||||
else
|
||||
# TODO i18n
|
||||
t :files_folder_name, 'files'
|
||||
if root_folders.select{|f| f.name == "files" }.empty?
|
||||
root_folders << context.folders.create(:name => "files", :full_name => "files", :workflow_state => "visible")
|
||||
end
|
||||
name = "files"
|
||||
end
|
||||
|
||||
root_folders = []
|
||||
|
||||
Folder.unique_constraint_retry do
|
||||
root_folder = context.folders.active.find_by_parent_folder_id_and_name(nil, name)
|
||||
root_folder ||= context.folders.create(:name => name, :full_name => name, :workflow_state => "visible")
|
||||
root_folders = [root_folder]
|
||||
end
|
||||
|
||||
root_folders
|
||||
end
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,17 @@
|
|||
class RemoveMultipleRootFolders < ActiveRecord::Migration
|
||||
tag :postdeploy
|
||||
self.transactional = false
|
||||
|
||||
def self.up
|
||||
DataFixup::RemoveMultipleRootFolders.run
|
||||
if connection.adapter_name =~ /\Apostgresql/i
|
||||
execute("CREATE UNIQUE INDEX CONCURRENTLY index_folders_on_context_id_and_context_type_for_root_folders ON folders(context_id, context_type) WHERE parent_folder_id IS NULL AND workflow_state != 'deleted'")
|
||||
end
|
||||
end
|
||||
|
||||
def self.down
|
||||
if connection.adapter_name =~ /\Apostgresql/i
|
||||
execute("DROP INDEX IF EXISTS index_folders_on_context_id_and_context_type_for_root_folders")
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
@ -0,0 +1,68 @@
|
|||
module DataFixup::RemoveMultipleRootFolders
|
||||
def self.run(opts = {})
|
||||
|
||||
limit = opts[:limit] || 1000
|
||||
|
||||
while (folders = Folder.find(
|
||||
:all, :conditions => ["workflow_state != ? AND parent_folder_id IS NULL", 'deleted'],
|
||||
:select => "context_id, context_type", :having => "count(*) > 1", :group => "context_id, context_type", :limit => limit
|
||||
)
|
||||
).any? do
|
||||
|
||||
context_types = folders.map(&:context_type).uniq
|
||||
|
||||
context_types.each do |context_type|
|
||||
|
||||
if context_type == "Course"
|
||||
root_folder_name = Folder::ROOT_FOLDER_NAME
|
||||
elsif context_type == "User"
|
||||
root_folder_name = Folder::MY_FILES_FOLDER_NAME
|
||||
else
|
||||
root_folder_name = "files"
|
||||
end
|
||||
|
||||
context_ids = folders.select{|f| f.context_type == context_type}.map(&:context_id)
|
||||
|
||||
root_folders = Folder.find(:all, :conditions => [
|
||||
"context_type = ? AND context_id IN (?) AND workflow_state != ? AND parent_folder_id IS NULL",
|
||||
context_type, context_ids, 'deleted'
|
||||
])
|
||||
|
||||
context_ids.each do |context_id|
|
||||
|
||||
context_root_folders = root_folders.select{|folder| folder.context_id == context_id}
|
||||
|
||||
main_root_folder = context_root_folders.select{|folder|
|
||||
folder.name == root_folder_name
|
||||
}.sort{|f1, f2|
|
||||
f2.attachments.count + f2.sub_folders.count <=> f1.attachments.count + f1.sub_folders.count
|
||||
}.first
|
||||
|
||||
if main_root_folder.nil?
|
||||
main_root_folder = Folder.create!(
|
||||
:context_type => context_type, :context_id => context_id,
|
||||
:name => root_folder_name, :full_name => root_folder_name, :workflow_state => "visible")
|
||||
context_root_folders << main_root_folder
|
||||
end
|
||||
|
||||
if context_root_folders.count > 1
|
||||
context_root_folders.each do |folder|
|
||||
unless folder.id == main_root_folder.id
|
||||
Folder.transaction do
|
||||
if folder.attachments.count > 0 || folder.sub_folders.count > 0
|
||||
Folder.update_all({:parent_folder_id => main_root_folder.id}, {:id => folder.id})
|
||||
else
|
||||
Folder.update_all({:workflow_state => 'deleted'}, {:id => folder.id})
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
end
|
||||
|
||||
end
|
||||
end
|
||||
|
||||
end
|
||||
end
|
||||
|
|
@ -0,0 +1,214 @@
|
|||
require File.expand_path(File.dirname(__FILE__) + '/../spec_helper.rb')
|
||||
require 'db/migrate/20130122193536_remove_multiple_root_folders'
|
||||
|
||||
|
||||
describe 'DataFixup::RemoveMultipleRootFolders' do
|
||||
self.use_transactional_fixtures = false
|
||||
|
||||
def get_root_folder_name(context)
|
||||
if context.is_a? Course
|
||||
root_folder_name = Folder::ROOT_FOLDER_NAME
|
||||
elsif context.is_a? User
|
||||
root_folder_name = Folder::MY_FILES_FOLDER_NAME
|
||||
else
|
||||
root_folder_name = "files"
|
||||
end
|
||||
root_folder_name
|
||||
end
|
||||
|
||||
before :each do
|
||||
RemoveMultipleRootFolders.down
|
||||
|
||||
@contexts = []
|
||||
|
||||
12.times do |x|
|
||||
case x % 4
|
||||
when 0
|
||||
context = course
|
||||
when 1
|
||||
context = user
|
||||
when 2
|
||||
context = group
|
||||
when 3
|
||||
context = Account.create!
|
||||
end
|
||||
@contexts << context
|
||||
end
|
||||
end
|
||||
|
||||
after :each do
|
||||
@contexts.each do |c|
|
||||
c.folders.each do |f|
|
||||
f.attachments.delete_all
|
||||
end
|
||||
Folder.delete_all(:id => c.folders.map(&:id))
|
||||
c.delete
|
||||
end
|
||||
RemoveMultipleRootFolders.up
|
||||
end
|
||||
|
||||
it "should remove extra root folders if they are empty" do
|
||||
empty_folders = []
|
||||
|
||||
@contexts.each do |context|
|
||||
Folder.root_folders(context)
|
||||
empty_folders << context.folders.create!(:name => "name1")
|
||||
empty_folders << context.folders.create!(:name => "name2")
|
||||
empty_folders << context.folders.create!(:name => "name3")
|
||||
|
||||
Folder.update_all({:parent_folder_id => nil}, {:context_type => context.class.to_s, :context_id => context.id})
|
||||
|
||||
Folder.count(:conditions => ["context_type = ? AND context_id = ? AND workflow_state != ? AND parent_folder_id IS NULL",
|
||||
context.class.to_s, context.id, 'deleted']
|
||||
).should == 4
|
||||
end
|
||||
|
||||
DataFixup::RemoveMultipleRootFolders.run(:limit => 2)
|
||||
|
||||
@contexts.each do |c|
|
||||
Folder.count(:conditions => ["context_type = ? AND context_id = ? AND workflow_state != ? AND parent_folder_id IS NULL",
|
||||
c.class.to_s, c.id, 'deleted']
|
||||
).should == 1
|
||||
end
|
||||
|
||||
empty_folders.each do |folder|
|
||||
folder.reload
|
||||
folder.workflow_state.should == 'deleted'
|
||||
end
|
||||
end
|
||||
|
||||
it "should move extra root folders to one root folder if they are not empty (either a sub-folder or attachment)" do
|
||||
extra_folders = []
|
||||
|
||||
@contexts.each do |context|
|
||||
Folder.root_folders(context)
|
||||
extra_folder1 = context.folders.create!(:name => "name1")
|
||||
extra_folders << extra_folder1
|
||||
extra_folder1.sub_folders.create!(:name => "name2", :context => context)
|
||||
|
||||
extra_folder2 = context.folders.create!(:name => "name1")
|
||||
extra_folders << extra_folder2
|
||||
a = extra_folder2.active_file_attachments.build
|
||||
a.context = context
|
||||
a.uploaded_data = default_uploaded_data
|
||||
a.save!
|
||||
|
||||
Folder.update_all({:parent_folder_id => nil}, {:id => [extra_folder1.id, extra_folder2.id]})
|
||||
|
||||
Folder.count(:conditions => ["context_type = ? AND context_id = ? AND workflow_state != ? AND parent_folder_id IS NULL",
|
||||
context.class.to_s, context.id, 'deleted']
|
||||
).should == 3
|
||||
end
|
||||
|
||||
DataFixup::RemoveMultipleRootFolders.run(:limit => 2)
|
||||
|
||||
@contexts.each do |c|
|
||||
Folder.count(:conditions => ["context_type = ? AND context_id = ? AND workflow_state != ? AND parent_folder_id IS NULL",
|
||||
c.class.to_s, c.id, 'deleted']
|
||||
).should == 1
|
||||
end
|
||||
|
||||
extra_folders.each do |folder|
|
||||
folder.reload
|
||||
folder.workflow_state.should_not == 'deleted'
|
||||
folder.parent_folder_id.should == Folder.root_folders(folder.context).first.id
|
||||
end
|
||||
end
|
||||
|
||||
it "should move extra root folders to the root folder with the most content" do
|
||||
extra_folders = []
|
||||
|
||||
empty_root_folder_ids = []
|
||||
root_folder_ids_with_content = []
|
||||
|
||||
@contexts.each do |context|
|
||||
root_folder_name = get_root_folder_name(context)
|
||||
|
||||
empty_root_folder = context.folders.create!(:name => root_folder_name)
|
||||
empty_root_folder_ids << empty_root_folder.id
|
||||
|
||||
root_folder_with_content = context.folders.create!(:name => root_folder_name)
|
||||
root_folder_with_content.sub_folders.create!(:name => "name2", :context => context)
|
||||
root_folder_ids_with_content << root_folder_with_content.id
|
||||
|
||||
extra_folder1 = context.folders.create!(:name => "name1")
|
||||
extra_folders << extra_folder1
|
||||
extra_folder1.sub_folders.create!(:name => "name2", :context => context)
|
||||
|
||||
extra_folder2 = context.folders.create!(:name => "name1")
|
||||
extra_folders << extra_folder2
|
||||
a = extra_folder2.active_file_attachments.build
|
||||
a.context = context
|
||||
a.uploaded_data = default_uploaded_data
|
||||
a.save!
|
||||
|
||||
Folder.update_all({:parent_folder_id => nil}, {:id => [extra_folder1.id, extra_folder2.id]})
|
||||
|
||||
Folder.count(:conditions => ["context_type = ? AND context_id = ? AND workflow_state != ? AND parent_folder_id IS NULL",
|
||||
context.class.to_s, context.id, 'deleted']
|
||||
).should == 4
|
||||
end
|
||||
|
||||
DataFixup::RemoveMultipleRootFolders.run(:limit => 2)
|
||||
|
||||
@contexts.each do |c|
|
||||
Folder.count(:conditions => ["context_type = ? AND context_id = ? AND workflow_state != ? AND parent_folder_id IS NULL",
|
||||
c.class.to_s, c.id, 'deleted']
|
||||
).should == 1
|
||||
end
|
||||
|
||||
extra_folders.each do |folder|
|
||||
folder.reload
|
||||
folder.workflow_state.should_not == 'deleted'
|
||||
folder.parent_folder_id.should == Folder.root_folders(folder.context).first.id
|
||||
empty_root_folder_ids.include?(folder.parent_folder_id).should be_false
|
||||
root_folder_ids_with_content.include?(folder.parent_folder_id).should be_true
|
||||
end
|
||||
end
|
||||
|
||||
it "should create a new root folder with the proper name if it doesn't exist already" do
|
||||
extra_folders = []
|
||||
|
||||
@contexts.each do |context|
|
||||
root_folder_name = get_root_folder_name(context)
|
||||
context.folders.find_by_name(root_folder_name).should be_nil
|
||||
|
||||
extra_folder1 = context.folders.create!(:name => "name1")
|
||||
extra_folders << extra_folder1
|
||||
extra_folder1.sub_folders.create!(:name => "name2", :context => context)
|
||||
|
||||
extra_folder2 = context.folders.create!(:name => "name1")
|
||||
extra_folders << extra_folder2
|
||||
a = extra_folder2.active_file_attachments.build
|
||||
a.context = context
|
||||
a.uploaded_data = default_uploaded_data
|
||||
a.save!
|
||||
|
||||
Folder.update_all({:parent_folder_id => nil}, {:id => [extra_folder1.id, extra_folder2.id]})
|
||||
|
||||
Folder.count(:conditions => ["context_type = ? AND context_id = ? AND workflow_state != ? AND parent_folder_id IS NULL",
|
||||
context.class.to_s, context.id, 'deleted']
|
||||
).should == 3
|
||||
end
|
||||
|
||||
DataFixup::RemoveMultipleRootFolders.run(:limit => 2)
|
||||
|
||||
@contexts.each do |c|
|
||||
Folder.count(:conditions => ["context_type = ? AND context_id = ? AND workflow_state != ? AND parent_folder_id IS NULL",
|
||||
c.class.to_s, c.id, 'deleted']
|
||||
).should == 1
|
||||
|
||||
root_folder_name = get_root_folder_name(c)
|
||||
c.folders.find_by_name(root_folder_name).should_not be_nil
|
||||
end
|
||||
|
||||
extra_folders.each do |folder|
|
||||
folder.reload
|
||||
folder.workflow_state.should_not == 'deleted'
|
||||
folder.parent_folder_id.should_not be_nil
|
||||
|
||||
root_folder_name = get_root_folder_name(folder.context)
|
||||
folder.parent_folder.name.should == root_folder_name
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
@ -28,30 +28,28 @@ describe Folder do
|
|||
end
|
||||
|
||||
it "should infer its full name if it has a parent folder" do
|
||||
f = @course.folders.create!(:name => "root")
|
||||
f.full_name.should eql("root")
|
||||
f = Folder.root_folders(@course).first
|
||||
f.full_name.should eql("course files")
|
||||
child = f.active_sub_folders.build(:name => "child")
|
||||
child.context = @course
|
||||
child.save!
|
||||
child.parent_folder.should eql(f)
|
||||
child.full_name.should eql("root/child")
|
||||
child.full_name.should eql("course files/child")
|
||||
grandchild = child.sub_folders.build(:name => "grandchild")
|
||||
grandchild.context = @course
|
||||
grandchild.save!
|
||||
grandchild.full_name.should eql("root/child/grandchild")
|
||||
grandchild.full_name.should eql("course files/child/grandchild")
|
||||
great_grandchild = grandchild.sub_folders.build(:name => "great_grandchild")
|
||||
great_grandchild.context = @course
|
||||
great_grandchild.save!
|
||||
great_grandchild.full_name.should eql("root/child/grandchild/great_grandchild")
|
||||
child.parent_folder = nil
|
||||
child.save!
|
||||
child.reload
|
||||
child.parent_folder.should be_nil
|
||||
child.full_name.should eql("child")
|
||||
great_grandchild.full_name.should eql("course files/child/grandchild/great_grandchild")
|
||||
|
||||
grandchild.parent_folder = f
|
||||
grandchild.save!
|
||||
grandchild.reload
|
||||
grandchild.full_name.should eql("child/grandchild")
|
||||
grandchild.full_name.should eql("course files/grandchild")
|
||||
great_grandchild.reload
|
||||
great_grandchild.full_name.should eql("child/grandchild/great_grandchild")
|
||||
great_grandchild.full_name.should eql("course files/grandchild/great_grandchild")
|
||||
end
|
||||
|
||||
it "should not allow recursive folder structures" do
|
||||
|
|
@ -100,10 +98,10 @@ describe Folder do
|
|||
|
||||
it "should implement the not_locked scope correctly" do
|
||||
not_locked = [
|
||||
@course.folders.create!(:name => "not locked 1"),
|
||||
@course.folders.create!(:name => "not locked 2", :locked => false),
|
||||
@course.folders.create!(:name => "not locked 3", :lock_at => 1.days.from_now),
|
||||
@course.folders.create!(:name => "not locked 4", :lock_at => 2.days.ago, :unlock_at => 1.days.ago)
|
||||
Folder.root_folders(@course).first,
|
||||
@course.folders.create!(:name => "not locked 1", :locked => false),
|
||||
@course.folders.create!(:name => "not locked 2", :lock_at => 1.days.from_now),
|
||||
@course.folders.create!(:name => "not locked 3", :lock_at => 2.days.ago, :unlock_at => 1.days.ago)
|
||||
]
|
||||
locked = [
|
||||
@course.folders.create!(:name => "locked 1", :locked => true),
|
||||
|
|
@ -113,4 +111,14 @@ describe Folder do
|
|||
@course.folders.map(&:id).sort.should == (not_locked + locked).map(&:id).sort
|
||||
@course.folders.not_locked.map(&:id).sort.should == (not_locked).map(&:id).sort
|
||||
end
|
||||
|
||||
it "should not create multiple root folders for a course" do
|
||||
pending('spec requires postgres index') unless Folder.connection.adapter_name == 'PostgreSQL'
|
||||
|
||||
@course.folders.create!(:name => Folder::ROOT_FOLDER_NAME, :full_name => Folder::ROOT_FOLDER_NAME, :workflow_state => 'visible')
|
||||
lambda { @course.folders.create!(:name => Folder::ROOT_FOLDER_NAME, :full_name => Folder::ROOT_FOLDER_NAME, :workflow_state => 'visible') }.should raise_error
|
||||
|
||||
@course.reload
|
||||
@course.folders.count.should == 1
|
||||
end
|
||||
end
|
||||
|
|
|
|||
Loading…
Reference in New Issue