diff --git a/app/models/folder.rb b/app/models/folder.rb index 2a5dff9b87b..1afb670fb7c 100644 --- a/app/models/folder.rb +++ b/app/models/folder.rb @@ -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 diff --git a/db/migrate/20130122193536_remove_multiple_root_folders.rb b/db/migrate/20130122193536_remove_multiple_root_folders.rb new file mode 100644 index 00000000000..f825007e5a3 --- /dev/null +++ b/db/migrate/20130122193536_remove_multiple_root_folders.rb @@ -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 diff --git a/lib/data_fixup/remove_multiple_root_folders.rb b/lib/data_fixup/remove_multiple_root_folders.rb new file mode 100644 index 00000000000..4d33abe0310 --- /dev/null +++ b/lib/data_fixup/remove_multiple_root_folders.rb @@ -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 \ No newline at end of file diff --git a/spec/migrations/remove_multiple_root_folders_spec.rb b/spec/migrations/remove_multiple_root_folders_spec.rb new file mode 100644 index 00000000000..9168826787c --- /dev/null +++ b/spec/migrations/remove_multiple_root_folders_spec.rb @@ -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 \ No newline at end of file diff --git a/spec/models/folder_spec.rb b/spec/models/folder_spec.rb index 1ef04d69bdf..dffbef3d531 100644 --- a/spec/models/folder_spec.rb +++ b/spec/models/folder_spec.rb @@ -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