From a5d8bb8041e8ee50e7f8dddd5c9687e33a91b51b Mon Sep 17 00:00:00 2001 From: Jacob Fugal Date: Thu, 14 Nov 2013 14:17:35 -0700 Subject: [PATCH] limit fake_arel caveats to rails2 refs CNVS-4704 fake_arel gets us most of the way to rails3's arel, but with some caveats. the workarounds (mostly) work in both, but it'll be nice to not have them crufting around when we're on rails3. so only use the workarounds in CANVAS_RAILS2 branches. Change-Id: Ia1f5cfa6b10f83cdb8d9a6e397b12401d180118d Reviewed-on: https://gerrit.instructure.com/26331 Tested-by: Jenkins Reviewed-by: Cody Cutrer Product-Review: Jacob Fugal QA-Review: Jacob Fugal --- app/models/conversation.rb | 20 +++++++++++++++----- app/models/course.rb | 19 +++++++++++++------ app/models/discussion_topic.rb | 4 ++-- app/models/notification_policy.rb | 11 +++++++---- app/models/stream_item.rb | 11 +++++++++-- config/initializers/active_record.rb | 6 +++--- lib/api/v1/assignment_override.rb | 18 ++++++++++++------ lib/messageable_user/calculator.rb | 17 +++++++++++------ 8 files changed, 72 insertions(+), 34 deletions(-) diff --git a/app/models/conversation.rb b/app/models/conversation.rb index bfa209d0aa3..4b713f9a220 100644 --- a/app/models/conversation.rb +++ b/app/models/conversation.rb @@ -668,9 +668,13 @@ class Conversation < ActiveRecord::Base ConversationMessageParticipant.joins(:conversation_message). where(:conversation_messages => { :conversation_id => self.id }). delete_all - # bare scoped call avoids HasManyAssociation's delete_all, which loads - # all records in Rails 2 - self.conversation_messages.scoped.delete_all + if CANVAS_RAILS2 + # bare scoped call avoids HasManyAssociation's delete_all, which loads + # all records in Rails 2 + self.conversation_messages.scoped.delete_all + else + self.conversation_messages.delete_all + end end end @@ -760,8 +764,14 @@ class Conversation < ActiveRecord::Base def delete_for_all stream_item.try(:destroy_stream_item_instances) - # bare scoped call avoid Rails 2 HasManyAssociation loading all objects - shard.activate { conversation_message_participants.scoped.delete_all } + shard.activate do + if CANVAS_RAILS2 + # bare scoped call avoid Rails 2 HasManyAssociation loading all objects + conversation_message_participants.scoped.delete_all + else + conversation_message_participants.delete_all + end + end conversation_participants.with_each_shard { |scope| scope.scoped.delete_all; nil } end diff --git a/app/models/course.rb b/app/models/course.rb index a9bfb99862e..71b952c2a20 100644 --- a/app/models/course.rb +++ b/app/models/course.rb @@ -536,9 +536,12 @@ class Course < ActiveRecord::Base end def instructors_in_charge_of(user_id) - section_ids = current_enrollments. - where(:course_id => self, :user_id => user_id). - where("course_section_id IS NOT NULL").pluck(:course_section_id).uniq # TODO: with real Rails 3, this uniq can become part of the scope + scope = current_enrollments. + where(:course_id => self, :user_id => user_id). + where("course_section_id IS NOT NULL") + section_ids = CANVAS_RAILS2 ? + scope.pluck(:course_section_id).uniq : + scope.uniq.pluck(:course_section_id) participating_instructors.restrict_to_sections(section_ids) end @@ -1609,9 +1612,13 @@ class Course < ActiveRecord::Base end def resubmission_for(asset) - # without the scoped, Rails 2 will try to do an update_all instead (due to - # the association) - asset.ignores.where(:purpose => 'grading', :permanent => false).scoped.delete_all + if CANVAS_RAILS2 + # without the scoped, Rails 2 will try to do an update_all instead (due + # to the association) + asset.ignores.where(:purpose => 'grading', :permanent => false).scoped.delete_all + else + asset.ignores.where(:purpose => 'grading', :permanent => false).delete_all + end instructors.each(&:touch) end diff --git a/app/models/discussion_topic.rb b/app/models/discussion_topic.rb index 2965a6c1410..61b242c4da0 100644 --- a/app/models/discussion_topic.rb +++ b/app/models/discussion_topic.rb @@ -497,8 +497,8 @@ class DiscussionTopic < ActiveRecord::Base end def user_ids_who_have_posted_and_admins - # TODO: In Rails 3, you can use uniq and pluck together - ids = DiscussionEntry.active.select(:user_id).uniq.where(:discussion_topic_id => self).map(&:user_id) + scope = DiscussionEntry.active.select(:user_id).uniq.where(:discussion_topic_id => self) + ids = CANVAS_RAILS2 ? scope.map(&:user_id) : scope.pluck(:user_id) ids += self.context.admin_enrollments.active.pluck(:user_id) if self.context.respond_to?(:admin_enrollments) ids end diff --git a/app/models/notification_policy.rb b/app/models/notification_policy.rb index 315c985def8..278cec9b4a2 100644 --- a/app/models/notification_policy.rb +++ b/app/models/notification_policy.rb @@ -113,14 +113,17 @@ class NotificationPolicy < ActiveRecord::Base # entry. If other than than, create or update the entry. NotificationPolicy.transaction do notifications.each do |notification_id| - # can't use hash syntax for the where cause Rails 2 will try to call communication_channels= for the - # or_initialize portion if CANVAS_RAILS2 + # can't use hash syntax for the where cause Rails 2 will try to + # call communication_channels= for the or_initialize portion p = NotificationPolicy.includes(:communication_channel).where("communication_channels.user_id=?", user). find_or_initialize_by_communication_channel_id_and_notification_id(params[:channel_id], notification_id) else - p = NotificationPolicy.joins(:communication_channel).where(:communication_channels => { :user_id => user }). - find_or_initialize_by_communication_channel_id_and_notification_id(params[:channel_id], notification_id) + p = NotificationPolicy.joins(:communication_channel).where( + communication_channels: {user_id: user}, + communication_channel_id: params[:channel_id], + notification_id: notification_id + ).find_or_initialize end # Set the frequency and save p.frequency = frequency diff --git a/app/models/stream_item.rb b/app/models/stream_item.rb index a9ededfafe8..d6b009d59d7 100644 --- a/app/models/stream_item.rb +++ b/app/models/stream_item.rb @@ -408,7 +408,14 @@ class StreamItem < ActiveRecord::Base public def destroy_stream_item_instances - # bare scoped call avoid Rails 2 HasManyAssociation loading all objects - self.stream_item_instances.with_each_shard { |scope| scope.scoped.delete_all; nil } + self.stream_item_instances.with_each_shard do |scope| + if CANVAS_RAILS2 + # bare scoped call avoid Rails 2 HasManyAssociation loading all objects + scope.scoped.delete_all + else + scope.delete_all + end + nil + end end end diff --git a/config/initializers/active_record.rb b/config/initializers/active_record.rb index 441b9297945..4fb9ef19d92 100644 --- a/config/initializers/active_record.rb +++ b/config/initializers/active_record.rb @@ -808,10 +808,10 @@ class ActiveRecord::Base end if Rails.version < '4' - if CANVAS_RAILS3 - scope :none, lambda { where("?", false) } - else + if CANVAS_RAILS2 named_scope :none, lambda { where("?", false) } + else + scope :none, lambda { where("?", false) } end end end diff --git a/lib/api/v1/assignment_override.rb b/lib/api/v1/assignment_override.rb index 81a6823b3c2..6536610a1c4 100644 --- a/lib/api/v1/assignment_override.rb +++ b/lib/api/v1/assignment_override.rb @@ -196,12 +196,18 @@ module Api::V1::AssignmentOverride end unless defunct_student_ids.empty? - # on Rails 2, the delete_all will do an update_all - # if we don't put the scoped in. weird. - override.assignment_override_students. - where(:user_id => defunct_student_ids.to_a). - scoped. - delete_all + if CANVAS_RAILS2 + # on Rails 2, the delete_all will do an update_all + # if we don't put the scoped in. weird. + override.assignment_override_students. + where(:user_id => defunct_student_ids.to_a). + scoped. + delete_all + else + override.assignment_override_students. + where(:user_id => defunct_student_ids.to_a). + delete_all + end end end diff --git a/lib/messageable_user/calculator.rb b/lib/messageable_user/calculator.rb index ff029c39b40..2a8960b3443 100644 --- a/lib/messageable_user/calculator.rb +++ b/lib/messageable_user/calculator.rb @@ -572,6 +572,7 @@ class MessageableUser :common_role_column => 'enrollments.type' }.merge(options) scope = base_scope(options) + scope = scope.joins("INNER JOIN enrollments ON enrollments.user_id=users.id") unless CANVAS_RAILS2 # see below enrollment_conditions = self.class.enrollment_conditions(options) if enrollment_conditions @@ -581,12 +582,16 @@ class MessageableUser scope = scope.where('?', false) end - # this comes after the conditional join on courses that needs - # enrollments, because AREL is going to swap the order for some reason. - # if this came first (e.g. immediately after base_scope), then the join - # on courses would show up first in the SQL, which could make the - # database sad. - scope.joins("INNER JOIN enrollments ON enrollments.user_id=users.id") + if CANVAS_RAILS2 + # this comes after the conditional join on courses that needs + # enrollments, because fake_arel is going to swap the order for some + # reason. if this came first (e.g. immediately after base_scope), then + # the join on courses would show up first in the SQL, which could make + # the database sad. + scope = scope.joins("INNER JOIN enrollments ON enrollments.user_id=users.id") + end + + scope end # further restricts the enrollment scope to users whose enrollment is