improve acts_as_list

ccutrer · ccutrer · commit 65da18174ebe · 2013-12-17T19:55:43.000Z
fixes CNVS-9622 basically, stop with the assumption that lists must be contiguous, so that we don't have to keep compacting them all the time. allows for far simpler queries adding things to lists. test plan: * regression test on lists - files, context modules, assignments, etc. * basically make sure that adding new items puts them in the same spot as it did before (bottom of list?), and that reordering still works Change-Id: I31c9ad4ed9b7db2b23e032617d4a01611c8e3c03 Reviewed-on: https://gerrit.instructure.com/26709 Tested-by: Jenkins <jenkins@instructure.com> Reviewed-by: Jacob Fugal <jacob@instructure.com> QA-Review: August Thornton <august@instructure.com> Product-Review: Cody Cutrer <cody@instructure.com>
diff --git a/app/controllers/account_authorization_configs_controller.rb b/app/controllers/account_authorization_configs_controller.rb
@@ -320,7 +320,7 @@ def create
       account_config = @account.account_authorization_configs.create!(data)
 
       if position.present?
-        account_config.insert_at(position)
+        account_config.insert_at(position.to_i)
         account_config.save!
       end
 
@@ -356,7 +356,7 @@ def update
     aac.update_attributes(data)
 
     if position.present?
-      aac.insert_at(position)
+      aac.insert_at(position.to_i)
       aac.save!
     end
 
diff --git a/app/controllers/assessment_questions_controller.rb b/app/controllers/assessment_questions_controller.rb
@@ -28,8 +28,6 @@ def create
       params[:assessment_question][:form_question_data] ||= params[:question]
       @question = @bank.assessment_questions.build(params[:assessment_question])
       if @question.with_versioning(&:save)
-        @question.insert_at_bottom
-
         render json: question_json(@question, @current_user, session, [:assessment_question])
       else
         render :json => @question.errors, :status => :bad_request
@@ -46,8 +44,6 @@ def update
       params[:assessment_question][:form_question_data] ||= params[:question]
       @question.edited_independent_of_quiz_question
       if @question.with_versioning { @question.update_attributes(params[:assessment_question]) }
-        @question.ensure_in_list
-
         render json: question_json(@question, @current_user, session, [:assessment_question])
       else
         render :json => @question.errors, :status => :bad_request
diff --git a/app/controllers/context_module_items_api_controller.rb b/app/controllers/context_module_items_api_controller.rb
@@ -363,10 +363,11 @@ def update
       @tag.url = params[:module_item][:external_url] if %w(ExternalUrl ContextExternalTool).include?(@tag.content_type) && params[:module_item][:external_url]
       @tag.indent = params[:module_item][:indent] if params[:module_item][:indent]
       @tag.new_tab = value_to_boolean(params[:module_item][:new_tab]) if params[:module_item][:new_tab]
-      if target_module_id = params[:module_item][:module_id]
+      if (target_module_id = params[:module_item][:module_id]) && target_module_id.to_i != @tag.context_module_id
         target_module = @context.context_modules.find_by_id(target_module_id)
         return render :json => {:message => "invalid module_id"}, :status => :bad_request unless target_module
         old_module = @context.context_modules.find(@tag.context_module_id)
+        @tag.remove_from_list
         @tag.context_module = target_module
         if req_index = old_module.completion_requirements.find_index { |req| req[:id] == @tag.id }
           old_module.completion_requirements_will_change!
@@ -511,7 +512,7 @@ def set_position
     return true unless @tag && params[:module_item][:position]
 
     @tag.reload
-    if @tag.insert_at_position(params[:module_item][:position], @tag.context_module.content_tags.not_deleted)
+    if @tag.insert_at(params[:module_item][:position].to_i)
       # see ContextModulesController#reorder_items
       @tag.touch_context_module
       ContentTag.update_could_be_locked(@tag.context_module.content_tags)
diff --git a/app/controllers/context_modules_api_controller.rb b/app/controllers/context_modules_api_controller.rb
@@ -375,7 +375,7 @@ def destroy
   def set_position
     return true unless @module && params[:module][:position]
 
-    if @module.insert_at_position(params[:module][:position], @context.context_modules.not_deleted)
+    if @module.insert_at(params[:module][:position].to_i)
       # see ContextModulesController#reorder
       @context.touch
       @context.context_modules.not_deleted.each{|m| m.save_without_touching_context }
diff --git a/app/controllers/eportfolios_controller.rb b/app/controllers/eportfolios_controller.rb
@@ -120,11 +120,7 @@ def destroy
   def reorder_categories
     @portfolio = Eportfolio.find(params[:eportfolio_id])
     if authorized_action(@portfolio, @current_user, :update)
-      order = params[:order].split(",")
-      order.each do |id|
-        category = @portfolio.eportfolio_categories.find_by_id(id) if id.present?
-        category.move_to_bottom if category
-      end
+      @portfolio.eportfolio_categories.build.update_order(params[:order].split(","))
       respond_to do |format|
         format.json { render :json => @portfolio.eportfolio_categories.map{|c| [c.id, c.position]}, :status => :ok }
       end
@@ -135,11 +131,7 @@ def reorder_entries
     @portfolio = Eportfolio.find(params[:eportfolio_id])
     if authorized_action(@portfolio, @current_user, :update)
       @category = @portfolio.eportfolio_categories.find(params[:eportfolio_category_id])
-      order = params[:order].split(",")
-      order.each do |id|
-        entry = @category.eportfolio_entries.find_by_id(id) if id.present?
-        entry.move_to_bottom if entry
-      end
+      @category.eportfolio_entries.build.update_order(params[:order].split(","))
       respond_to do |format|
         format.json { render :json => @portfolio.eportfolio_entries.map{|c| [c.id, c.position]}, :status => :ok }
       end
diff --git a/app/models/announcement.rb b/app/models/announcement.rb
@@ -31,9 +31,7 @@ class Announcement < DiscussionTopic
   validates_presence_of :context_type
   validates_presence_of :message
 
-  acts_as_list scope: %q{context_id = '#{context_id}' AND
-                         context_type = '#{context_type}' AND
-                         type = 'Announcement'}
+  acts_as_list scope: { context: self, type: 'Announcement' }
 
   def validate_draft_state_change
     old_draft_state, new_draft_state = self.changes['workflow_state']
diff --git a/app/models/assessment_question.rb b/app/models/assessment_question.rb
@@ -25,7 +25,7 @@ class AssessmentQuestion < ActiveRecord::Base
   attr_accessor :initial_context
   belongs_to :assessment_question_bank, :touch => true
   simply_versioned :automatic => false
-  acts_as_list :scope => :assessment_question_bank_id
+  acts_as_list :scope => :assessment_question_bank
   before_validation :infer_defaults
   after_save :translate_links_if_changed
   validates_length_of :name, :maximum => maximum_string_length, :allow_nil => true
diff --git a/app/models/assignment.rb b/app/models/assignment.rb
@@ -124,7 +124,7 @@ def external_tool?
   validates_length_of :allowed_extensions, :maximum => maximum_long_text_length, :allow_nil => true, :allow_blank => true
   validate :frozen_atts_not_altered, :if => :frozen?, :on => :update
 
-  acts_as_list :scope => :assignment_group_id
+  acts_as_list :scope => :assignment_group
   simply_versioned :keep => 5
   sanitize_field :description, Instructure::SanitizeField::SANITIZE
   copy_authorized_links( :description) { [self.context, nil] }
diff --git a/app/models/assignment_group.rb b/app/models/assignment_group.rb
@@ -22,13 +22,13 @@ class AssignmentGroup < ActiveRecord::Base
 
   attr_accessible :name, :rules, :assignment_weighting_scheme, :group_weight, :position, :default_assignment_name
   attr_readonly :context_id, :context_type
-  acts_as_list :scope => 'assignment_groups.context_type = #{connection.quote(context_type)} AND assignment_groups.context_id = #{context_id} AND assignment_groups.workflow_state <> \'deleted\''
+  belongs_to :context, :polymorphic => true
+  acts_as_list scope: { context: self, workflow_state: 'available' }
   has_a_broadcast_policy
 
   has_many :assignments, :order => 'position, due_at, title', :dependent => :destroy
   has_many :active_assignments, :class_name => 'Assignment', :conditions => ['assignments.workflow_state != ?', 'deleted'], :order => 'assignments.position, assignments.due_at, assignments.title'
 
-  belongs_to :context, :polymorphic => true
   validates_presence_of :context_id, :context_type, :workflow_state
   validates_length_of :rules, :maximum => maximum_text_length, :allow_nil => true, :allow_blank => true
   validates_length_of :default_assignment_name, :maximum => maximum_string_length, :allow_nil => true
diff --git a/app/models/communication_channel.rb b/app/models/communication_channel.rb
@@ -39,7 +39,7 @@ class CommunicationChannel < ActiveRecord::Base
   validate :not_otp_communication_channel, :if => lambda { |cc| cc.path_type == TYPE_SMS && cc.retired? && !cc.new_record? }
   validates_presence_of :access_token_id, if: lambda { |cc| cc.path_type == TYPE_PUSH }
 
-  acts_as_list :scope => :user_id
+  acts_as_list :scope => :user
 
   has_a_broadcast_policy
 
diff --git a/app/models/context_module.rb b/app/models/context_module.rb
@@ -23,7 +23,7 @@ class ContextModule < ActiveRecord::Base
   belongs_to :context, :polymorphic => true
   has_many :context_module_progressions, :dependent => :destroy
   has_many :content_tags, :dependent => :destroy, :order => 'content_tags.position, content_tags.title'
-  acts_as_list :scope => 'context_modules.context_type = #{connection.quote(context_type)} AND context_modules.context_id = #{context_id} AND context_modules.workflow_state <> \'deleted\''
+  acts_as_list scope: { context: self, workflow_state: ['active', 'unpublished'] }
   
   serialize :prerequisites
   serialize :completion_requirements
diff --git a/app/models/course_profile.rb b/app/models/course_profile.rb
@@ -1,3 +1,3 @@
 class CourseProfile < Profile
-  acts_as_list :scope => :root_account_id
+  acts_as_list :scope => :root_account
 end
diff --git a/app/models/discussion_topic.rb b/app/models/discussion_topic.rb
@@ -62,9 +62,7 @@ module DiscussionTypes
 
   sanitize_field :message, Instructure::SanitizeField::SANITIZE
   copy_authorized_links(:message) { [self.context, nil] }
-  acts_as_list scope: %q{context_id = #{context_id} AND
-                         context_type = '#{context_type}' AND
-                         pinned = TRUE}
+  acts_as_list scope: { context: self, pinned: true }
 
   before_create :initialize_last_reply_at
   before_save :default_values
diff --git a/app/models/pseudonym.rb b/app/models/pseudonym.rb
@@ -67,7 +67,7 @@ def require_password?
     password_changed? || (send(crypted_password_field).blank? && sis_ssha.blank?) || @require_password
   end
 
-  acts_as_list :scope => :user_id
+  acts_as_list :scope => :user
 
   set_broadcast_policy do |p|
     p.dispatch :confirm_registration
diff --git a/config/initializers/acts_as_list.rb b/config/initializers/acts_as_list.rb
diff --git a/spec/apis/v1/context_module_items_api_spec.rb b/spec/apis/v1/context_module_items_api_spec.rb
@@ -345,7 +345,8 @@
         tag.position.should == 3
 
         tags.each{|t| t.reload}
-        tags.map(&:position).should == [1, 2, 4, 5]
+        # 2 is deleted; 3 is the new one, that displaced the others to 4-6
+        tags.map(&:position).should == [1, 4, 5, 6]
       end
 
       it "should set completion requirement" do
diff --git a/spec/lib/acts_as_list_spec.rb b/spec/lib/acts_as_list_spec.rb
@@ -25,14 +25,15 @@
       a2 = attachment_model
       a3 = attachment_model
       a4 = attachment_model
-      Attachment.expects(:update_all).with("position=CASE WHEN id=#{a2.id} THEN 1 WHEN id=#{a3.id} THEN 2 WHEN id=#{a1.id} THEN 3 WHEN id=#{a4.id} THEN 4 ELSE 0 END", anything)
-      Attachment.expects(:update_all).with("position=CASE WHEN id=#{a3.id} THEN 1 WHEN id=#{a1.id} THEN 2 WHEN id=#{a2.id} THEN 3 WHEN id=#{a4.id} THEN 4 ELSE 0 END", anything)
+      list = a1.list_scope
       a1.update_order([a2.id, a3.id, a1.id])
+      list.pluck(:id).should == [a2.id, a3.id, a1.id, a4.id]
       a1.update_order(["SELECT now()", a3.id, "evil stuff"])
+      list.pluck(:id).should == [a3.id, a2.id, a1.id, a4.id]
     end
   end
 
-  describe "#insert_at_position" do
+  describe "#insert_at" do
     before :each do
       course
       @module_1 = @course.context_modules.create!(:name => "another module")
@@ -45,33 +46,19 @@
     it "should insert in the position correctly" do
       @modules.map(&:position).should == [1, 2, 3]
 
-      @module_1.insert_at_position(3).should == true
+      @module_1.insert_at(3).should == true
       @modules.each{|m| m.reload}
       @modules.map(&:position).should == [3, 1, 2]
 
-      @module_2.insert_at_position(2).should == true
+      @module_2.insert_at(2).should == true
       @modules.each{|m| m.reload}
       @modules.map(&:position).should == [3, 2, 1]
 
-      @module_3.insert_at_position(3).should == true
+      @module_3.insert_at(3).should == true
       @modules.each{|m| m.reload}
       @modules.map(&:position).should == [2, 1, 3]
 
-      @module_1.insert_at_position(1).should == true
-      @modules.each{|m| m.reload}
-      @modules.map(&:position).should == [1, 2, 3]
-    end
-
-    it "should handle positions outside range" do
-      @module_2.insert_at_position(-10).should == false # do nothing
-      @modules.each{|m| m.reload}
-      @modules.map(&:position).should == [1, 2, 3]
-
-      @module_3.insert_at_position(0).should == false # do nothing
-      @modules.each{|m| m.reload}
-      @modules.map(&:position).should == [1, 2, 3]
-
-      @module_1.insert_at_position(4).should == false # do nothing
+      @module_1.insert_at(1).should == true
       @modules.each{|m| m.reload}
       @modules.map(&:position).should == [1, 2, 3]
     end
@@ -81,8 +68,7 @@
     it "should order null positions last" do
       course
       module_1 = @course.context_modules.create :name => 'one'
-      module_1.position = nil
-      module_1.save!
+      ContextModule.where(id: module_1).update_all(position: nil)
       module_2 = @course.context_modules.create :name => 'two'
       module_2.position = 1
       module_2.save!
@@ -102,7 +88,7 @@
       @course.context_modules.map{|m| [m.id, m.position]}.should eql [[module_1.id, 1], [module_2.id, 2]]
     end
 
-    it "should leave gaps alone" do
+    it "should consolidate gaps" do
       course
       module_1 = @course.context_modules.create :name => 'one'
       module_1.position = 1
@@ -111,7 +97,7 @@
       module_2.position = 3
       module_2.save!
       module_1.fix_position_conflicts
-      @course.context_modules.map{|m| [m.id, m.position]}.should eql [[module_1.id, 1], [module_2.id, 3]]
+      @course.context_modules.map{|m| [m.id, m.position]}.should eql [[module_1.id, 1], [module_2.id, 2]]
     end
   end
 end
diff --git a/vendor/plugins/acts_as_list/README b/vendor/plugins/acts_as_list/README
@@ -17,7 +17,7 @@ Example
   end
 
   todo_list.first.move_to_bottom
-  todo_list.last.move_higher
+  todo_list.last.insert_at(2)
 
 
 Copyright (c) 2007 David Heinemeier Hansson, released under the MIT license
diff --git a/vendor/plugins/acts_as_list/lib/active_record/acts/list.rb b/vendor/plugins/acts_as_list/lib/active_record/acts/list.rb
diff --git a/vendor/plugins/acts_as_list/test/list_test.rb b/vendor/plugins/acts_as_list/test/list_test.rb
